Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix C headers including both ddog_Vec_U8 and ddog_prof_Vec_U8 #83

Merged
merged 3 commits into from
Jan 11, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jan 6, 2023

What does this PR do?:

In #73, we changed a number of types, functions and enums to be properly scoped inside or outside namespaces.

But while integrating the 1.0.1 release in Ruby, I noticed that by mistake I forgot to properly configure the renaming for Vec_U8, which resulted in two copies of Vec_U8 being emitted in the header files: ddog_Vec_U8 and ddof_prof_Vec_U8, with some APIs using one and others using the other.

This PR fixes the configuration.

Motivation:

Having two copies of the same struct is not correct and is really annoying for clients as well -- I temporarily needed to duplicate a helper on the Ruby profiler to have variants for both types (since often we don't even use a pointer but the struct directly, you can't even cast between them).

Additional Notes:

Here's the diff for the generated headers before/after this change:

diff -uwr old-include/datadog/common.h build-linux/x86_64-linux/include/datadog/common.h
--- old-include/datadog/common.h	2023-01-06 10:57:32.730644531 +0000
+++ build-linux/x86_64-linux/include/datadog/common.h	2023-01-06 10:58:41.954571430 +0000
@@ -106,16 +106,6 @@
  */
 typedef struct ddog_prof_Exporter_Request ddog_prof_Exporter_Request;

-/**
- * Holds the raw parts of a Rust Vec; it should only be created from Rust,
- * never from C.
- */
-typedef struct ddog_prof_Vec_U8 {
-  const uint8_t *ptr;
-  uintptr_t len;
-  uintptr_t capacity;
-} ddog_prof_Vec_U8;
-
 typedef enum ddog_prof_Exporter_NewResult_Tag {
   DDOG_PROF_EXPORTER_NEW_RESULT_OK,
   DDOG_PROF_EXPORTER_NEW_RESULT_ERR,
@@ -128,7 +118,7 @@
       struct ddog_prof_Exporter *ok;
     };
     struct {
-      struct ddog_prof_Vec_U8 err;
+      struct ddog_Vec_U8 err;
     };
   };
 } ddog_prof_Exporter_NewResult;
@@ -205,7 +195,7 @@
       struct ddog_HttpStatus http_response;
     };
     struct {
-      struct ddog_prof_Vec_U8 err;
+      struct ddog_Vec_U8 err;
     };
   };
 } ddog_prof_Exporter_SendResult;
@@ -399,7 +389,7 @@
 typedef struct ddog_prof_EncodedProfile {
   struct ddog_Timespec start;
   struct ddog_Timespec end;
-  struct ddog_prof_Vec_U8 buffer;
+  struct ddog_Vec_U8 buffer;
   struct ddog_prof_ProfiledEndpointsStats *endpoints_stats;
 } ddog_prof_EncodedProfile;

@@ -415,7 +405,7 @@
       struct ddog_prof_EncodedProfile ok;
     };
     struct {
-      struct ddog_prof_Vec_U8 err;
+      struct ddog_Vec_U8 err;
     };
   };
 } ddog_prof_Profile_SerializeResult;
diff -uwr old-include/datadog/profiling.h build-linux/x86_64-linux/include/datadog/profiling.h
--- old-include/datadog/profiling.h	2023-01-06 10:57:32.730644531 +0000
+++ build-linux/x86_64-linux/include/datadog/profiling.h	2023-01-06 10:58:41.954571430 +0000
@@ -224,7 +224,7 @@

 void ddog_prof_Profile_SerializeResult_drop(struct ddog_prof_Profile_SerializeResult _result);

-DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_prof_Vec_U8 *vec);
+DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_Vec_U8 *vec);

 /**
  * Resets all data in `profile` except the sample types and period. Returns

This is technically a breaking change for users of the C bindings, since a type gets removed (even if the type was never supposed to have existed).

Since #80 is already bumping the libdatadog version to 2.0.0, I suggest only merging this PR after that one, and including it in the 2.0.0 release.

How to test the change?:

This change doesn't actually affect the compiled library, only the header files. Thus, the only way of noticing and testing this change, is to compile something that includes the headers and validate that only ddog_Vec_U8 gets used.

**What does this PR do?**:

In #73, we changed a number of types, functions and enums to be
properly scoped inside or outside namespaces.

But while integrating the 1.0.1 release in Ruby, I noticed that by
mistake I forgot to properly configure the renaming for `Vec_U8`,
which resulted in two copies of `Vec_U8` being emitted in the header
files: `ddog_Vec_U8` and `ddof_prof_Vec_U8`, with some APIs using one
and others using the other.

This PR fixes the configuration.

**Motivation**:

Having two copies of the same struct is not correct and is really
annoying for clients as well -- I temporarily needed to duplicate
a helper on the Ruby profiler to have variants for both types
(since often we don't even use a pointer but the struct directly,
you can't even cast between them).

**Additional Notes**:

Here's the diff for the generated headers before/after this change:

```diff
diff -uwr old-include/datadog/common.h build-linux/x86_64-linux/include/datadog/common.h
--- old-include/datadog/common.h	2023-01-06 10:57:32.730644531 +0000
+++ build-linux/x86_64-linux/include/datadog/common.h	2023-01-06 10:58:41.954571430 +0000
@@ -106,16 +106,6 @@
  */
 typedef struct ddog_prof_Exporter_Request ddog_prof_Exporter_Request;

-/**
- * Holds the raw parts of a Rust Vec; it should only be created from Rust,
- * never from C.
- */
-typedef struct ddog_prof_Vec_U8 {
-  const uint8_t *ptr;
-  uintptr_t len;
-  uintptr_t capacity;
-} ddog_prof_Vec_U8;
-
 typedef enum ddog_prof_Exporter_NewResult_Tag {
   DDOG_PROF_EXPORTER_NEW_RESULT_OK,
   DDOG_PROF_EXPORTER_NEW_RESULT_ERR,
@@ -128,7 +118,7 @@
       struct ddog_prof_Exporter *ok;
     };
     struct {
-      struct ddog_prof_Vec_U8 err;
+      struct ddog_Vec_U8 err;
     };
   };
 } ddog_prof_Exporter_NewResult;
@@ -205,7 +195,7 @@
       struct ddog_HttpStatus http_response;
     };
     struct {
-      struct ddog_prof_Vec_U8 err;
+      struct ddog_Vec_U8 err;
     };
   };
 } ddog_prof_Exporter_SendResult;
@@ -399,7 +389,7 @@
 typedef struct ddog_prof_EncodedProfile {
   struct ddog_Timespec start;
   struct ddog_Timespec end;
-  struct ddog_prof_Vec_U8 buffer;
+  struct ddog_Vec_U8 buffer;
   struct ddog_prof_ProfiledEndpointsStats *endpoints_stats;
 } ddog_prof_EncodedProfile;

@@ -415,7 +405,7 @@
       struct ddog_prof_EncodedProfile ok;
     };
     struct {
-      struct ddog_prof_Vec_U8 err;
+      struct ddog_Vec_U8 err;
     };
   };
 } ddog_prof_Profile_SerializeResult;
diff -uwr old-include/datadog/profiling.h build-linux/x86_64-linux/include/datadog/profiling.h
--- old-include/datadog/profiling.h	2023-01-06 10:57:32.730644531 +0000
+++ build-linux/x86_64-linux/include/datadog/profiling.h	2023-01-06 10:58:41.954571430 +0000
@@ -224,7 +224,7 @@

 void ddog_prof_Profile_SerializeResult_drop(struct ddog_prof_Profile_SerializeResult _result);

-DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_prof_Vec_U8 *vec);
+DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_Vec_U8 *vec);

 /**
  * Resets all data in `profile` except the sample types and period. Returns
```

This is technically a breaking change for users of the C bindings,
since a type gets removed (even if the type was never supposed to
have existed).

Since #80 is already bumping the libdatadog version to 2.0.0,
I suggest only merging this PR after that one, and including it
in the 2.0.0 release.

**How to test the change?**:

This change doesn't actually affect the compiled library, only the
header files. Thus, the only way of noticing and testing this change,
is to compile something that includes the headers and validate that
only `ddog_Vec_U8` gets used.
@ivoanjo ivoanjo requested a review from a team as a code owner January 6, 2023 11:23
@ivoanjo ivoanjo merged commit 7f66aae into main Jan 11, 2023
@morrisonlevi morrisonlevi deleted the ivoanjo/fix-prof-vec-u8 branch May 10, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants