Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Add helper for creating CharSlice from a string literal #36

Merged
merged 4 commits into from
Mar 21, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Mar 17, 2022

What does this PR do?

Add a ddprof_ffi_CharSlice_from_literal macro to ffi.h that makes it easy to write something like

const ddprof_ffi_Label label = {
  .key = ddprof_ffi_CharSlice_from_literal("language"),
  .str = ddprof_ffi_CharSlice_from_literal("php"),
}

Motivation

It looks like everyone that uses libddprof has a variant of this macro, so I think it makes sense to just have it in ffi.h.

This specific implementation is built to only work with string literals, aka there's no accidental/unexpected strlen going on.

How to test the change?

Use macro to create CharSlices and confirm everything still works as expected.

It looks like everyone that uses libddprof has a variant of this
macro, so I think it makes sense to just have it in `ffi.h`.

This specific implementation is built to only work with string
literals, aka there's no accidental/unexpected  `strlen` going on.
@@ -12,6 +12,10 @@ style = "both"
no_includes = true
sys_includes = ["stdbool.h", "stddef.h", "stdint.h"]

after_includes = """

#define ddprof_ffi_CharSlice_from_literal(string) ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof("" string) - 1})"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion !
But I find sizeof dangerous:

const char* s = "foo";
...
ddprof_ffi_CharSlice_from_literal(s);

will construct an incorrect slice.
Using strlen would avoid such errors and strlen call would be optimized away by the compiler for string literals.
I am also not a big fan of macros, you could use static inline function:

static inline ddprof_ffi_CharSlice ddprof_ffi_CharSlice_from_literal(const char*s) {
    return (ddprof_ffi_CharSlice){.ptr=s, .len=strlen(s); };
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion !
But I find sizeof dangerous:

const char* s = "foo";
...
ddprof_ffi_CharSlice_from_literal(s);

will construct an incorrect slice.

Actually, by design, it won't. It will give you a compile error :D :D

That's because of the .ptr = "" string instead of .ptr = string -- you can only char *foo = "do " "this " "if " "every " "part " "is " "a " "literal";.

To be fair the error you get is not a great one, but at least it does barf at the usage.

We could have both ddprof_ffi_CharSlice_from_literal as well as a strlen version ddprof_ffi_CharSlice_from_char (?), although I do somewhat dislike that you may be doing strlen without realizing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you got me thinking that the error message could be improved. I've pushed d20aff4 that adds a nice comment which incidentally gets shown by the compiler when you try to pass in a char*:

foo.c: In function 'endpoint_from':
foo.c:133:27: error: expected '}' before 's'
   byte_slice_from_literal(s);
                           ^
foo.c:15:113: note: in definition of macro 'byte_slice_from_literal'
   /* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof(string) - 1})
                                                                                                                ^~~~~~
foo.c:15:91: note: to match this '{'
   /* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof(string) - 1})

This macro relies on a weird trick to fail compilation when you pass
in a char* and not a literal (the trick is that
`char *foo = "" "foo";` is valid but `char *bar; char *foo = "" bar;`
is not), but the error message would be kinda cryptic.

Since most compilers show the macro when the compilation fails,
I've added a nice comment which gets shown by compilers:

```
foo.c: In function 'endpoint_from':
foo.c:133:27: error: expected '}' before 's'
   byte_slice_from_literal(s);
                           ^
foo.c:15:113: note: in definition of macro 'byte_slice_from_literal'
   /* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof(string) - 1})
                                                                                                                ^~~~~~
foo.c:15:91: note: to match this '{'
   /* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof(string) - 1})
```
@@ -12,6 +12,11 @@ style = "both"
no_includes = true
sys_includes = ["stdbool.h", "stddef.h", "stdint.h"]

after_includes = """

#define ddprof_ffi_CharSlice_from_literal(string) \\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any objection to DDPROF_FFI_CHARSLICE_C instead? It's shorter, in upper-case like macros should be, and uses the _C convention which is used in standard macros for integer literals eg INT64_C(1000000000000).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and pushed a commit. If you object, feel free to revert. I was just trying to speed up the speculative happy path.

@ivoanjo
Copy link
Member Author

ivoanjo commented Mar 21, 2022

Happy with your change @morrisonlevi .

Going ahead and merging -- @nsavoire let me know if you're still unconvinced, happy to revisit or even revert if we think this is not the right direction 😄 🙇

@ivoanjo ivoanjo merged commit 99cc62f into main Mar 21, 2022
@ivoanjo ivoanjo deleted the ivoanjo/add-charslice-helper branch March 21, 2022 09:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants