Add helper for creating CharSlice from a string literal#36
Conversation
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.
|
|
||
| after_includes = """ | ||
|
|
||
| #define ddprof_ffi_CharSlice_from_literal(string) ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof("" string) - 1})""" |
There was a problem hiding this comment.
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); };
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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})
```
|
|
||
| after_includes = """ | ||
|
|
||
| #define ddprof_ffi_CharSlice_from_literal(string) \\ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
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 😄 🙇 |
3 participants
What does this PR do?
Add a
ddprof_ffi_CharSlice_from_literalmacro toffi.hthat makes it easy to write something likeMotivation
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
strlengoing on.How to test the change?
Use macro to create CharSlices and confirm everything still works as expected.