remove constexpr on level_string_views to fix compilation on C++17 fr…

[stevenlunt]

…om addition of set_string_view

[sjanel]

@gabime Please merge this, spdlog is not usable in C++17 without it ;) Thanks in advance!

[gabime]

will release new version

[gv-me]

Hi, this removes the constexpr declaration for log levels, which re-adds the problems caused in issue #1791.

Static initialization will now cause problems again, now that the level names are not initialized reliably before entering main()

I cannot see the utility in saving a bit of compilation time while changing the log levels (which was the feature that caused the removal of constexpr here) while sacrificing the runtime stability and initialization order.

@gabime even then if people want this, a compile option be added to choose between either constexpr log levels vs the ability to change them at runtime?

[gabime]

@gabime even then if people want this, a compile option be added to choose between either constexpr log levels vs the ability to change them at runtime?

Sounds right. But what about pre cpp 17 ?

[gv-me]

for pre cpp 17, we can't declare string_view as constexpr anyway, due to the constructor that's being used, which is does not have constexpr until cpp 17.

[gabime]

This means this option should be offered only if cpp17 or newer is detected. PR is welcome.

[gv-me]

yes, that's correct. I will take a look how it can be done, maybe in cmake.

[stevenlunt]

I am fine with a compile-time option for c++17 and beyond. it is still better than it was before, where I had to patch the spdlog package in order to change the strings.

[gv-me]

@stevenlunt can you elaborate on what do you mean by "patching the package"?

If you use spdlog as header only lib, you only have define SPDLOG_LEVEL_NAMES and SPDLOG_SHORT_LEVEL_NAMES before you include spdlog.
If you compile spdlog from source, you only have to add the definition at compile time.
If you include spdlog in your project using cmake as add_subdirectory, you can define it in cmake as target_compile_definitions.

I cannot see the real use for setting the level names in a function in c++ code, when you should not be changing them often anyway.

[stevenlunt]

i've tried updating the recipe to allow you to pass an option with the level names but it is very difficult if not impossible to convince CMake to define something like { "trace", "dbg", ...

It would be better if rather than one preprocessor token there were instead separate tokens for each level string. this way it would be much more straightforward to override one or more of them

[gv-me]

I just tested once with cmake command line and this works fine for me

cmake . -DSPDLOG_LEVEL_NAMES='{'\"mytrace\",\"mydebug\",\"myinfo\",\"mywarn\",\"mycritical\",\"myerror\",\"myoff\"'}'

in my CMakeLists I added

add_subdirectory(./path/to/spdlog spdlog)
if(SPDLOG_LEVEL_NAMES)
    target_compile_definitions(spdlog PUBLIC SPDLOG_LEVEL_NAMES=${SPDLOG_LEVEL_NAMES})
endif()

The lines after add_subdirectory can actually be added to spdlog CMakeLists.txt as well to directly make it work with no changes in user projects.

But you are right that if there was a token for each level it would look better.

[stevenlunt]

i was trying to pass options to conan create, something like:

conan create . 1.8.5@third-party/stable -o spdlog:level_name_debug=DBG -o spdlog:level_name_info=INF -o spdlog:level_name_warning=WRN -o spdlog:level_name_error=ERR

[gv-me]

I have not used conan options before, would these then be passed through as CMake variables from conan?

[stevenlunt]

it does not seem possible through conan options to pass a macro through to gcc like {"string","string",...}

[gv-me]

would something like this help perhaps?
conan-io/conan#4754 (comment)

[stevenlunt]

i will create a new pull request to revert my changes from this PR and add separate string #defines for the individual level names

[stevenlunt]

#1948