Constexpr unit symbol

[jbbjarnason]

Add unit_symbol_view to generate unit_symbol during compilation.

Update iterator when adding to the provided buffer.

[mpusz]

I am sorry to reject this PR 😢

I do not think that exposing interfaces with static, fixed storage is a good idea. Guessing the number of characters is always wrong (either too big or too small). Also, it is not thread-safe.

A user can either use unit_symbol to get a compile-time std::string (no allocation at runtime) or can provide its own contiguous storage and call unit_symbol_to (as you did in your PR):

std::cout << unit_symbol(m / s) << "\n";

If you need std::string_view why not use:

static constexpr std::string txt = unit_symbol(m / s);
const std::string_view view = txt;
std::cout << view << "\n";

[mpusz]

Thanks for raising this issue. I will fix it on the mainline ASAP.

[jbbjarnason]

I am sorry to reject this PR 😢

I do not think that exposing interfaces with static, fixed storage is a good idea. Guessing the number of characters is always wrong (either too big or too small). Also, it is not thread-safe.

A user can either use unit_symbol to get a compile-time std::string (no allocation at runtime) or can provide its own contiguous storage and call unit_symbol_to (as you did in your PR):

std::cout << unit_symbol(m / s) << "\n";

If you need std::string_view why not use:

static constexpr std::string txt = unit_symbol(m / s);
const std::string_view view = txt;
std::cout << view << "\n";

Sad that it was rejected, because the library does not have a working interface to get unit_symbol at compile time. Or at least I am unable to make it work.

This static constexpr std::string txt = unit_symbol(m / s); is invalid since you cannot use heap allocated storage such as std::string as buffer, example: https://godbolt.org/z/cndKMTM6r
std::string does however work with string literals.

And tried your example:

static constexpr std::string foo = unit_symbol(kilometre);

Error:

[1/5] Building CXX object test/unit_test/static/CMakeFiles/foo.dir/unit_test.cpp.o
FAILED: test/unit_test/static/CMakeFiles/foo.dir/unit_test.cpp.o 
/usr/bin/cmake -E env CCACHE_DIRECT=1 CCACHE_NO_DEPEND=1 CCACHE_BASEDIR=/home/jonb/Projects/units /usr/bin/ccache /usr/bin/c++ -DFMT_SHARED -DMP_UNITS_USE_LIBFMT=1 -Dgsl_CONFIG_CONTRACT_CHECKING_AUDIT -I/home/jonb/Projects/units/src/core/include -I/home/jonb/Projects/units/src/core-io/include -I/home/jonb/Projects/units/src/core-fmt/include -I/home/jonb/Projects/units/src/systems/angular/include -I/home/jonb/Projects/units/src/systems/isq/include -I/home/jonb/Projects/units/src/systems/iec80000/include -I/home/jonb/Projects/units/src/systems/si/include -I/home/jonb/Projects/units/src/systems/isq_angle/include -I/home/jonb/Projects/units/src/systems/natural/include -I/home/jonb/Projects/units/src/systems/cgs/include -I/home/jonb/Projects/units/src/systems/hep/include -I/home/jonb/Projects/units/src/systems/iau/include -I/home/jonb/Projects/units/src/systems/imperial/include -I/home/jonb/Projects/units/src/systems/international/include -I/home/jonb/Projects/units/src/systems/typographic/include -I/home/jonb/Projects/units/src/systems/usc/include -I/home/jonb/Projects/units/src/utility/include -g -std=gnu++23 -fdiagnostics-color=always -Wall -Wextra -Wpedantic -Wshadow -Wnon-virtual-dtor -Wold-style-cast -Wcast-align -Wunused -Woverloaded-virtual -Wcast-qual -Wconversion -Wsign-conversion -Wnull-dereference -Wformat=2 -Wdangling-else -Wdouble-promotion -Wmisleading-indentation -Wduplicated-cond -Wduplicated-branches -Wlogical-op -Werror -MD -MT test/unit_test/static/CMakeFiles/foo.dir/unit_test.cpp.o -MF test/unit_test/static/CMakeFiles/foo.dir/unit_test.cpp.o.d -o test/unit_test/static/CMakeFiles/foo.dir/unit_test.cpp.o -c /home/jonb/Projects/units/test/unit_test/static/unit_test.cpp
../test/unit_test/static/unit_test.cpp:546:57: error: ‘std::__cxx11::basic_string<char>{std::__cxx11::basic_string<char>::_Alloc_hider{((char*)(& foo.std::__cxx11::basic_string<char>::<anonymous>.std::__cxx11::basic_string<char>::<unnamed union>::_M_local_buf))}, 2, std::__cxx11::basic_string<char>::<unnamed union>{char [16]{'k', 'm', 0}}}’ is not a constant expression
  546 | static constexpr std::string foo = unit_symbol(kilometre);
      |                                                         ^
[4/5] Building CXX object test/unit_test/static/CMakeFiles/foo.dir/isq_test.cpp.o
ninja: build stopped: subcommand failed.

But yes I will use unit_symbol_to, I just need the iterator fixed like you mention will be done.

[mpusz]

Yes, I admit this can be improved. However, the solution with a static array of a fixed size is not the right solution.

[chiphogg]

If you want, you can take some inspiration from Au's StringConstant: https://github.com/aurora-opensource/au/blob/main/au/utility/string_constant.hh

Here are our tests, to give you a feel for how it works: https://github.com/aurora-opensource/au/blob/main/au/utility/test/string_constant_test.cc

detail::as_char_array() shows how we can expose a character array of just-the-right-size for each unit, and we use this in our unit_label(U) function: https://github.com/aurora-opensource/au/blob/a57588019be67eb46a816a9d88183cd78d325dbb/au/unit_of_measure.hh#L794-L797

[JohelEGP]

expose a character array of just-the-right-size

You can also use lefticus/tools mentioned at #337 (comment).

[mpusz]

@jbbjarnason it would be great if you could proceed with the hints above. In case you are not interested or do not have time for it, could you please submit an issue for this problem referring to the above two messages as a possible solution?

[jbbjarnason]

@mpusz I have updated the branch according to the suggestions by @JohelEGP and @chiphogg.
For reference, jbbjarnason@5ac1dd4

Could you maybe reopen the PR so we could get updates here?

[mpusz]

Reopened.

Thanks a lot for your efforts! I will look into this soon.

[mpusz]

Thanks a lot! This worked in most of our compilers but, in my opinion, was too "hacky". It would be hard to defend such an interface and implementation in the C++ Committee. I just submitted a different solution that will hopefully resolve your issues.

Thanks again for raising this and proposing a resolution!