Switch to vformat_to

[sylveon]

Drive-by: reduce the amount of occurences of #ifdef SPDLOG_USE_STD_FORMAT

[gabime]

That’ s great. Thanks. Makes everything much cleaner.
There is small issue that now the logger.h includes the fmt_helper.h file and this might affect the compile times everywhere (it was on,ly included from “-inl.h” files, meaning included only when compiling the lib rather than from user code). Might be worth to place the new to_string in common.h instead.

[sylveon]

I had only included that in there for to_string_view, which isn't really needed to begin with, so I went and removed that include from logger.h to return to manually building the string_view_t

[gabime]

Returning with std::move is problematic since it prohibits copy elision optimization and compiler is allowed to move anyway (see https://stackoverflow.com/questions/17473753/c11-return-value-optimization-or-move).

So I think your original version in this pr (just return by value, no references or moves) is probably the best after all. The compiler would optimize this easily anyway and the code is simplest. sorry for the confusion.

[sylveon]

Calling fmt_helper::to_string in itself already kills NRVO.
Returning by std::move is only harmful when we do return a local variable by value, because it kills NRVO.
NRVO does not apply to returning parameters.

Until P1825R0, returning a rvalue reference does not move by default - we need to explicitly call std::move (this is because rvalue references are lvalues, thanks C++). This can be easily tested by trying to compile the following with GCC in C++17 mode or older (Clang backported P1825R0 to older standards). It won't compile:

std::unique_ptr<int> test(std::unique_ptr<int> &&ptr)
{
    return ptr;
}

Changing the return to use std::move(test) allows it to compile.

---

All of the above is also confirmed by looking at the assembly output of when using different to_string methods:

• test_no_func is the one with the shortest assembly output, because it allows NRVO to kick in
• Closely behind are test_rvalue_ref and test_rvalue_ref_non_moved, which do a few more movs before returning, because they call the move operator.
• test_by_val_moved_std_move and test_by_val_std_move do not copy, but are bigger than the prior two because it does a double move.
• test_const_ref (the one passing const std::string& does a copy. So does test_by_val_moved_copy and test_by_val_copy.

So the "most optimal" solution is to use #ifdef SPDLOG_USE_STD_FORMAT to only call fmt::to_string when needed, but the one that's less intrusive to source code while being almost as efficient is to keep std::string to_string(std::string&& str) with an explicit call to std::move inside to_string.

[gabime]

This all leads me to the think that ifdef should be used. Possibly by defining some macro like SPDLOG_BUF_TO_STRING(buf)

[sylveon]

Good idea that I didn't think of - done.

[gabime]

Merged. Thanks @sylveon