Skip to content

Introduce DDOG_CHARSLICE_C_BARE and undo change to DDOG_CHARSLICE_C helper#347

Merged
ivoanjo merged 4 commits intomainfrom
ivoanjo/introduce-charslice-bare-helper
Mar 11, 2024
Merged

Introduce DDOG_CHARSLICE_C_BARE and undo change to DDOG_CHARSLICE_C helper#347
ivoanjo merged 4 commits intomainfrom
ivoanjo/introduce-charslice-bare-helper

Conversation

@ivoanjo
Copy link
Copy Markdown
Member

@ivoanjo ivoanjo commented Mar 11, 2024

What does this PR do?

This PR introduces a new DDOG_CHARSLICE_C_BARE helper, as well as undoes the change to DDOG_CHARSLICE_C from
https://github.com/DataDog/libdatadog/pull/305/files#diff-5c00b9c49c81bf0d0a754e0bcc6e0add4fe849bf0ea4a4d1ebfac8f698150f0b

There are now two helpers: DDOG_CHARSLICE_C that includes the (ddog_CharSlice) cast and DDOG_CHARSLICE_C_BARE which doesn't.

Motivation

In a nutshell @bwoebi explained to me that on Windows there's times where you need to use (ddog_CharSlice) {.ptr = ..., .len = ...} and others where you need to use {.ptr = ..., .len = ...} and cannot include the cast.

Thus in #305 we tweaked the helper to never have (ddog_CharSlice), with the intention of using (ddog_CharSlice) DDOG_CHARSLICE_C(...) some times, and DDOG_CHARSLICE_C(...) the other times.

But... this had the downside of making some of our Ruby Profiler code really ugly, as we'd need to move from DDOG_CHARSLICE_C to (CharSlice) DDOG_CHARSLICE_C in a lot of cases.

Thus, this PR introduces one helper for each variant, so that we always have a nice user-friendly option for each case, and avoid needing to sprinkle casts.

Additional Notes

Arghhh C/C++ you always manage to be weird and annoying.

How to test the change?

I've tested this change by compiling the Ruby profiler to use this branch of libdatadog, and validating that it compiles successfully on Linux.

For Reviewers

  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

…C` helper

**What does this PR do?**

This PR introduces a new `DDOG_CHARSLICE_C_BARE` helper, as well
as undoes the change to `DDOG_CHARSLICE_C` from
https://github.com/DataDog/libdatadog/pull/305/files#diff-5c00b9c49c81bf0d0a754e0bcc6e0add4fe849bf0ea4a4d1ebfac8f698150f0b

There are now two helpers: `DDOG_CHARSLICE_C` that includes the
`(ddog_CharSlice)` cast and `DDOG_CHARSLICE_C_BARE` which doesn't.

**Motivation:**

In a nutshell @bwoebi explained to me that on Windows there's times
where you need to use `(ddog_CharSlice) {.ptr = ..., .len = ...}` and
others where you need to use `{.ptr = ..., .len = ...}` and cannot
include the cast.

Thus in #305 we tweaked the helper to never have `(ddog_CharSlice)`,
with the intention of using `(ddog_CharSlice) DDOG_CHARSLICE_C(...)`
some times, and `DDOG_CHARSLICE_C(...)` the other times.

But... this had the downside of making some of our Ruby Profiler code
really ugly, as we'd need to move from `DDOG_CHARSLICE_C` to
`(CharSlice) DDOG_CHARSLICE_C` in a lot of cases.

Thus, this PR introduces one helper for each variant, so that
we always have a nice user-friendly option for each case, and avoid
needing to sprinkle casts.

**Additional Notes:**

Arghhh C/C++ you always manage to be weird and annoying.

**How to test the change?**

I've tested this change by compiling the Ruby profiler to use this
branch of libdatadog, and validating that it compiles successfully
on Linux.
@ivoanjo ivoanjo requested a review from a team as a code owner March 11, 2024 12:01
@bwoebi
Copy link
Copy Markdown
Contributor

bwoebi commented Mar 11, 2024

You could define DDOG_CHARSLICE_C(str) as ((ddog_CharSlice)DDOG_CHARSLICE_C_BARE(str)) :-D

ivoanjo added a commit to DataDog/dd-trace-php that referenced this pull request Mar 11, 2024
**What does this PR do?**

This PR updated dd-trace-php to use the helpers that were changed/
introduced in DataDog/libdatadog#347 .

TL;DR: `DDOG_CHARSLICE_C` includes the cast to `(ddog_CharSlice)` to
all uses of `(ddog_CharSlice) DDOG_CHARSLICE_C` in the codebase were
converted to not have the cast; and `DDOG_CHARSLICE_C_BARE` does
not have the cast, so all uses of `DDOG_CHARSLICE_C` without the
cast in the codebase were converted to this one.

**Motivation:**

Update to match the latest libdatadog changes.

**Additional Notes:**

This is my first dd-trace-php contribution I think?

**How to test the change?**

I'm guessing CI should be able to validate this (?), not very
familiar with our testing setup.
ivoanjo added 3 commits March 11, 2024 13:55
CI was failing on windows with:

```
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(35,16): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(36,15): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(55,23): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(56,27): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(62,14): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(63,14): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(79,36): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(102,31): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(106,32): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(114,30): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(114,68): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(115,30): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(127,15): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(138,1): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\libdatadog\libdatadog\examples\ffi\exporter.cpp(141,1): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
```
@ivoanjo ivoanjo merged commit 242a0fe into main Mar 11, 2024
@ivoanjo ivoanjo deleted the ivoanjo/introduce-charslice-bare-helper branch March 11, 2024 14:25
@ivoanjo
Copy link
Copy Markdown
Member Author

ivoanjo commented Mar 11, 2024

You could define DDOG_CHARSLICE_C(str) as ((ddog_CharSlice)DDOG_CHARSLICE_C_BARE(str)) :-D

I decided to leave as-is, to hopefully simplify a bit error messages (e.g. when passing in a non-literal), but don't have a strong opinion so if you're really not convinced I can live with your suggestion as well ;)

bwoebi added a commit to DataDog/dd-trace-php that referenced this pull request Mar 11, 2024
…ers (#2565)

* Update to use `DDOG_CHARSLICE_C` and new `DDOG_CHARSLICE_C_BARE` helpers

This PR updated dd-trace-php to use the helpers that were changed/
introduced in DataDog/libdatadog#347 .

TL;DR: `DDOG_CHARSLICE_C` includes the cast to `(ddog_CharSlice)` to
all uses of `(ddog_CharSlice) DDOG_CHARSLICE_C` in the codebase were
converted to not have the cast; and `DDOG_CHARSLICE_C_BARE` does
not have the cast, so all uses of `DDOG_CHARSLICE_C` without the
cast in the codebase were converted to this one.

* Update libdatadog

---------

Co-authored-by: Bob Weinand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants