Changed to use strlen instead of sizeof to get the length of a string

[mwinterb]

Addresses #1063.

[mwinterb]

I used char_traits instead of strlen directly so that in header-only mode it would hopefully remain similar to the sizeof code because of the usages of __builtin_strlen.

[DanielaE]

This is pessimizing virtually all implementations. Honestly, I don't like such "solutions". If the original code is standards compliant, then it doesn't need a "fix" to get the value of a compile-time constant using C++98 syntax.

So, the question is: is the code conforming to the standard? Until recently all compilers that I used for testing (the compilers from the test-suite, gcc 7, gcc 8, clang 8, clang 9) were happily compiling the code (see below)

I've reported a similar to/the same problem as #1063 to Microsoft for investigation and a fix in their upcoming VS2019 compiler. This bug is closed by them now, pending the next release of their compiler toolset.

This is my result when compiling {fmt} with the clang 9 toolset in Visual Studio:

[vitaut]

Why not change the type of RESET_COLOR to fmt::string_view and use size()?

[mwinterb]

I agree. However: Were you testing in non-header-only mode? And explicitly the fmt::format function with one of the text styles? https://godbolt.org/z/K8GSR7
I don't see how the code in fmt/color.h can possibly determine the sizeof an object that is only defined in format.cc (in non-header-only mode). MSVC 2019 may have a separate bug, but it seems like it must fail because the definition of RESET_COLOR doesn't exist.

As an alternative fix, pre-declaring the size for RESET_COLOR will still allow for sizeof to be used unconditionally, but I assumed it was declared unsized to avoid the burden of ensuring the array is exactly the right length. But it's an easy test case to ensure that strlen and sizeof() -1 is the same result.

[mwinterb]

RESET_COLOR is currently passed directly to fputs, which needs a NTBS. Solvable, but more invasive. Also, the constructor for string_view isn't always constexpr, so for some users a dynamic initializer would be introduced. It's always constexpr for me, so I'm not personally worried, but I at least want it call it out for those stuck in a pre-C++17 world (MSVC only declares their char_traits::length as constexpr in C++17 mode, I don't know what libc++ and libstdc++ do).

In the header only case, O2 does optimize away the strlen. Which isn't ideal for those with "needs to be fast even with optimizations disabled" needs, but in this specific context, we're already paying for a call to memmove, and potentially a call to malloc.

I think pre-declaring the RESET_COLOR size as 5 with a test ensuring it's correct is probably the best case. I can put that out tomorrow if everyone agrees with that path and no one else gets to it first.

[vitaut]

I think pre-declaring the RESET_COLOR size as 5 with a test ensuring it's correct is probably the best case.

Sounds good to me.

[mwinterb]

See #1068.
Also, @DanielaE, sorry if my initial response was rude. I was a bit exasperated and confused and that may have come out as mean.

[DanielaE]

Nevermind - we're all nice kittens, aren't we? 😸

[mwinterb]

That's the goal!