Skip to main content
4 of 6
added 656 characters in body
ad absurdum
  • 894
  • 1
  • 10
  • 21

The accepted answer has done a fine job of reviewing the posted code. This answer will focus on the macros in this code.

In summary: the OP code should remove the SVLEN macro altogether and at least rewrite SV to improve type safety. There is discussion below about problems with SVLEN, why it should be removed, and suggestions for ways to improve SV.

To be clear: the OP macro, and the related macros in this answer, will only work with arguments that are string literals; OP has specified as much in the question and included code. It would be quite easy to get tripped up using these definitions.

It would be worth the OP's time to review whether SV needs to be a macro at all. Maybe it would be better to use a make_sv function that returns a str_view struct, allowing the caller to determine whether a string is backed by string literal storage or some other storage for which the caller is responsible. I would probably favor that direction.

As a minor nitpick: the str_view struct should use len instead of sz for the name of the field indicating the length of the string(view) in question. sz indicates a "size" and this is confusing nomenclature given that the size of the array backing a string and the length of that string are two different things.

Macros Should Be as Simple as Possible

The OP code defines two macros:

#define SVLEN(str) ((sizeof((str)) / sizeof((str)[0])) - sizeof((str)[0]))

#define SV(str) ((str_view){(str), SVLEN((str))})

The SVLEN macro can be made much simpler:

#define SVLEN_SIMPLE(str) (sizeof(str) - 1)

This SVLEN_SIMPLE version takes advantage of the fact that sizeof (char) is guaranteed to be 1 by the C Standard. Note that this implementation would not work for wide string literals, but the OP code is using pointers to char to access the string content so this seems fine here.

This use of sizeof to find the length of a string relies on the fact that as expressions string literals are array types, so sizeof yields the size of the array, not the size of the pointer to which the string literal would decay in most expressions. So SVLEN_SIMPLE("some string") would yield the length of "some string". But this would not work as desired:

char *some_string = "some string";
printf("%zu\n", SVLEN_SIMPLE(some_string));

It might be nice to do some rudimentary type-checking in this macro. A C compiler will concatenate adjacent string literal tokens during the translation phases, and you can take advantage of this by concatenating the macro argument provided by a caller with the empty string literal. If the caller provides something other than a string literal the compiler will complain.

#define SVLEN_SAFE(str) (sizeof(str "") - 1)

This version returns 0 when no argument is provided, i.e., SVLEN_SAFE() yields 0. This may or may not be desired behavior.

One might be tempted to surround the macro argument with empty string literals: #define SVLEN_DONT(str) (sizeof ("" str "") - 1). Don't do this: keep the macro as simple as possible. You only need one string literal and one non-string literal in the attempted concatenation to trigger a compiler error, and adding the second "" here even adds a new failure mode. With, e.g., SVLEN_DONT(-) there will be no error because "" - "" is a legal expression with integer type.

It is still possible to break the SVLEN_SAFE macro, e.g., SVLEN("this" - "that") would complete the concatenation with the empty string literal, and the expression "this" - "that" is a legal expression with integer type. If you feel like you need to handle this sort of bad input you can use the comma operator in the macro to check whether the input is an array.

#define SVLEN_QUESTIONABLE(str) ("" str "", (str)[0], sizeof(str) - 1)

The SVLEN_QUESTIONABLE macro expands to a sequence that first may fail to compile if str is not a string literal. Since this check does not catch all cases of bad input, a second check in the sequence attempts to access the first element of str, which may or may not be an array. If str is in fact a string literal then this compiles and the final part of the sequence computes the length of the string and that value is the final result of the sequence.

There at least one minor problem here. Using SVLEN_QUESTIONABLE can trigger compiler warnings since the left-hand operands of the comma expressions have no discernible effects. GCC issues these warnings when compiling with -Wall. Clang doesn't seem to mind at -Wall. These warnings are just added noise at compile time, and you might not want to disable -Wunused-value just for this since it is useful elsewhere. For those who like to compile with -Werror enabled this is a nuisance.

I'm not convinced that SVLEN_QUESTIONABLE is bullet-proof; it is at least slightly more robust than SVLEN_SAFE above, but is the added robustness worth the added complexity? Not in my opinion. SVLEN_SAFE does what we want, which is to enforce that the macro argument is a string literal in reasonable cases.

Or, Just Don't Use a Macro

But really, what is the point of SVLEN in the first place? Code should generally use strlen to find the length of strings. String literals are a special case for which it is sometimes useful to use the sizeof "something" - 1 idiom, but I don't see that making a macro to do this brings any real benefits; it just brings added complexity.

In my opinion, the SVLEN macro should not exist at all.

Improving the SV Macro

With SVLEN removed from the code, SV needs to be rewritten. The simple version would be:

#define SV_SIMPLE(str) ((str_view){(str), sizeof(str) - 1})

This version has the same type safety problems as the previous SVLEN_SIMPLE, i.e., a caller may provide something other than a string literal argument leading to unhappy results. As before, the situation can be improved by taking advantage of string literal concatenation.

Also note that rare unicorn platforms may exist where size_t is narrower than int. To guard against potential problems which may arise when a caller expects an unsigned result, unsigned math can be ensured by subtracting an unsigned integer constant from the result of sizeof. (@Davislor)

#define SV_SAFE(str) ((str_view){(str ""), sizeof(str) - 1U})

This usually fails to compile when the caller fails to provide a string literal argument. As with SVLEN_SAFE before, this version will compile SV_SAFE("this" - "that"), but unlike SVLEN_SAFE, SV_SAFE will (probably) generate a warning here (or an error if -Werror or similar is enabled) since this would attempt to assign an integer value (the value of "this" - "that") to a pointer variable (the s field of a str_view struct).

The previous SVLEN_SAFE() yielded 0, while SV_SAFE() is rejected outright. If you wanted to obtain similar behavior for SV_SAFE you could write:

#define SV_SAFE_ZERO(str) ((str_view){(str ""), sizeof(str "") - 1U})

SV_SAFE_ZERO() will yield a str_view struct initialized with a pointer to the empty string and the len field set to 0.

ad absurdum
  • 894
  • 1
  • 10
  • 21