Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example code how to use your library with the c++20 source_location #1959

Closed
theShmoo opened this issue May 31, 2021 · 23 comments
Closed

Example code how to use your library with the c++20 source_location #1959

theShmoo opened this issue May 31, 2021 · 23 comments

Comments

@theShmoo
Copy link

theShmoo commented May 31, 2021

I created an example code on how to use your library with the c++20 source_location instead of your macros:

#include <spdlog/spdlog.h>

#include <experimental/source_location>
#include <iostream>
#include <string_view>

namespace logging {
using source_location = std::experimental::source_location;
[[nodiscard]] constexpr auto get_log_source_location(
    const source_location &location) {
  return spdlog::source_loc{location.file_name(),
                            static_cast<std::int32_t>(location.line()),
                            location.function_name()};
}

struct format_with_location {
  std::string_view value;
  spdlog::source_loc loc;

  template <typename String>
  format_with_location(
      const String &s,
      const source_location &location = source_location::current())
      : value{s}, loc{get_log_source_location(location)} {}
};

template <typename... Args>
void warn(format_with_location fmt, Args &&...args) {
  spdlog::default_logger_raw()->log(fmt.loc, spdlog::level::warn, fmt.value,
                                    std::forward<Args>(args)...);
}
}  // namespace logging

int main() {
  spdlog::set_level(spdlog::level::trace);
  std::string message = "hello {}\n";
  auto value = 42;
  logging::warn(message, value);
}

https://godbolt.org/z/aqdYjo6nE

Because it took some time to fiddle with the type deduction I wanted to share this. Maybe you could add it to the documentation?

Thanks for your great library!

@gabime
Copy link
Owner

gabime commented May 31, 2021

Thanks. Very cool!
We might even want to add this to the spdlog code if c++20 is detected.

Why std::experimental instead of std::?

@theShmoo
Copy link
Author

Nice! That would be cool!

because clang 12.0.0 does not yet support std::source_location

@halfelf
Copy link

halfelf commented Jul 14, 2021

For the experimental issue, the example could be like this:

#ifdef __cpp_lib_source_location
#include <source_location>
using source_location = std::source_location;
#else
#include <experimental/source_location>
using source_location = std::experimental::source_location;
#endif

@ghost
Copy link

ghost commented Oct 3, 2021

@gabime This was neat and totally worked. I see that a 2.x branch is being developed. Is support for std::source_location being taken into consideration for it?

@gabime
Copy link
Owner

gabime commented Oct 3, 2021

Not yet. PRs are welcome.

@ghost
Copy link

ghost commented Oct 3, 2021

@gabime I missed this closed PR that already tried some approach #1884

I'm not actually sure the appropriate concerns have been addressed

@mkbn-consultancy
Copy link

Why in all print functions, spdlog prints only the file name and line, and does not print the function name (which is more important than the file name for my opinion)? I needed to add it manually to get it.

@theShmoo
Copy link
Author

theShmoo commented Feb 2, 2022

@mkbn-consultancy
Copy link

mkbn-consultancy commented Feb 16, 2022 via email

@Bluesman74
Copy link

@theShmoo, would you be able to update your example for MSVC please. I tried to by removing ::experimental and adding #include <format> but it complained about:

template <typename... Args>
void warn(format_with_location fmt, Args &&...args)
{
    spdlog::default_logger_raw()->log(fmt.loc, spdlog::level::warn, fmt.value,
        std::forward<Args>(args)...);  // NOSONAR
}

fmt.value
error C7595: 'fmt::v9::basic_format_string<char,int &>::basic_format_string': call to immediate function is not a constant

@theShmoo
Copy link
Author

sorry the snipped I posted is no longer like this in my code base and I don't use MSVC so you have to figure it out yourself. Maybe you can post a not working example via Godbolt and I can take a look.

@miyanyan
Copy link

miyanyan commented Aug 4, 2023

if removing ::experimental, i create a souce_location instead

#if defined(_MSC_VER)
#if __has_include(<yvals_core.h>)
#include <yvals_core.h>
#endif

#include <cstdint>

// copied from: https://github.com/microsoft/STL/blob/main/stl/inc/source_location
struct source_location
{
    static constexpr source_location current(const uint_least32_t _Line_ = __builtin_LINE(), const uint_least32_t _Column_ = __builtin_COLUMN(),
                                             const char* const _File_ = __builtin_FILE(), const char* const _Function_ = __builtin_FUNCTION()) noexcept
    {
        source_location _Result{};
        _Result._Line = _Line_;
        _Result._Column = _Column_;
        _Result._File = _File_;
        _Result._Function = _Function_;
        return _Result;
    }

    constexpr source_location() noexcept = default;

    constexpr uint_least32_t line() const noexcept { return _Line; }
    constexpr uint_least32_t column() const noexcept { return _Column; }
    constexpr const char* file_name() const noexcept { return _File; }
    constexpr const char* function_name() const noexcept { return _Function; }

private:
    uint_least32_t _Line{};
    uint_least32_t _Column{};
    const char* _File = "";
    const char* _Function = "";
};

#else

struct source_location
{
    // 14.1.2, source_location creation
    static constexpr source_location current(const char* __file = "unknown", const char* __func = "unknown", int __line = 0, int __col = 0) noexcept
    {
        source_location __loc;
        __loc._M_file = __file;
        __loc._M_func = __func;
        __loc._M_line = __line;
        __loc._M_col = __col;
        return __loc;
    }

    constexpr source_location() noexcept : _M_file("unknown"), _M_func(_M_file), _M_line(0), _M_col(0) {}

    // 14.1.3, source_location field access
    constexpr unsigned int line() const noexcept { return _M_line; }
    constexpr unsigned int column() const noexcept { return _M_col; }
    constexpr const char* file_name() const noexcept { return _M_file; }
    constexpr const char* function_name() const noexcept { return _M_func; }

private:
    const char* _M_file;
    const char* _M_func;
    unsigned int _M_line;
    unsigned int _M_col;
};

#endif

then

template<class T>
class with_source_location
{
public:
    template<class U>
    with_source_location(U&& u, source_location loc = source_location::current()) : m_value(std::forward<U>(u)), m_loc(loc)
    {
    }

    auto& value() { return m_value; }
    const auto& value() const { return m_value; }
    auto& location() { return m_loc; }
    const auto& location() const { return m_loc; }

private:
    T m_value;
    source_location m_loc;
};

@HaroldCC
Copy link

@theShmoo, would you be able to update your example for MSVC please. I tried to by removing ::experimental and adding #include <format> but it complained about:

template <typename... Args>
void warn(format_with_location fmt, Args &&...args)
{
    spdlog::default_logger_raw()->log(fmt.loc, spdlog::level::warn, fmt.value,
        std::forward<Args>(args)...);  // NOSONAR
}

fmt.value error C7595: 'fmt::v9::basic_format_string<char,int &>::basic_format_string': call to immediate function is not a constant
I test it on VisualStudio 2022, same error. So I use clang16 compiler, It same error. So I change spdlog header file core.h the constructor of basic_format_string, change FMT_CONSTEVAL to FMT_CONSTEXPR. It works. But I'm not sure if this is correct.

@theShmoo
Copy link
Author

https://godbolt.org/z/Y3G4PW47n

Hi!

Here is a working example

#include <spdlog/spdlog.h>

#include <iostream>
#include <source_location>
#include <string_view>

namespace logging {
using source_location = std::source_location;
[[nodiscard]] constexpr auto get_log_source_location(
    const source_location &location) {
    return spdlog::source_loc{location.file_name(),
                              static_cast<std::int32_t>(location.line()),
                              location.function_name()};
}

struct format_with_location {
    std::string_view value;
    spdlog::source_loc loc;

    template <typename String>
    format_with_location(const String &s, const source_location &location =
                                              source_location::current())
        : value{s}, loc{get_log_source_location(location)} {}
};

template <typename... Args>
void warn(format_with_location fmt, Args &&...args) {
    spdlog::default_logger_raw()->log(fmt.loc, spdlog::level::warn,
                                      fmt::runtime(fmt.value),
                                      std::forward<Args>(args)...);
}
}  // namespace logging

int main() {
    spdlog::set_level(spdlog::level::trace);
    std::string message = "hello {}\n";
    auto value = 42;
    logging::warn(message, value);
}

@Bluesman74
Copy link

@theShmoo Thanks so much, I've just tried it in VS2022 and it works great.

@gabime
Copy link
Owner

gabime commented Sep 3, 2023

I began investigating this issue for version 2.x. One problem is that the compiler embeds the location information for all code that uses spdlog, even if the user doesn't need this feature or only logs conditionally. It seems that the only way to avoid this is by defining a flag before including spdlog, such as SPDLOG_ENABLE_SOURCE_LOCATION or SPDLOG_DISABLE_SOURCE_LOCATION.
Another option could be to add a specialized API, for example, logger->info_loc() or something similar.

Any thoughts?

@bebuch
Copy link

bebuch commented Oct 9, 2023

@gabime I assume performance is why source_location should not be enabled by default? Turning it on will pretty surely require a bit more memory on the stack. However, it may well be just as fast anyway. That would have to be benchmarked.

Case it really is slower, I would prefer a SPDLOG_DISABLE_SOURCE_LOCATION. In my personal option simplicity by default is more importent then performance by default.

For a specialized API I would prefer something like this:

  • info with source_location
  • thin_info without source_location

What you are suggesting is:

  • info without source_location
  • info_loc with source_location

This would result in info often being used by mistake instead of info_loc. That way important information is accidentally lost.

Conversely, users who need maximum performance and definitely don't need source_location would probably be mindful to use thin_info. And if they do get it wrong, they only lose some runtime performance, but no information.

All in all, I would prefer the Specialized API approach if including source_location is the default API and without source_location is the specialized API.

@gabime
Copy link
Owner

gabime commented Oct 9, 2023

v2.x branch already supports source location. you need to define #define SPDLOG_SOURCE_LOCATION before including spdlog.h.

Please try it out and feedback. It requires to compile with cpp 20(-DCMAKE_CXX_STANDARD=20) although it seems to even work with some compilers with cpp 17 (which is the default standard in spdlog v2.x).

@bebuch
Copy link

bebuch commented Oct 10, 2023

@gabime I've seen that, and I already use it ;-)

However, I don't like the current solution with the macro to turn on. A define to switch off would be better in my opinion. I would find a specialized API for log without location even better.

In the ideal case, a benchmark could show that the location as default parameter does not bring any performance disadvantages if it is not used. In this case, you could do without a define solution and a specialized API.

@gabime
Copy link
Owner

gabime commented Oct 10, 2023

Since embedding all source locations into the product binary is probably not desirable for many products, I decided to disable it by default and enable only if the macro is defined. Seems easy enough, instead of replicating the whole API.

Feel free to suggest other means. As long as by default it is turned off.

@bebuch
Copy link

bebuch commented Oct 10, 2023

Yeah okay, unwanted source location strings in (commercial) binaries are a strong argument.

@Bluesman74
Copy link

@theShmoo Thanks so much, I've just tried it in VS2022 and it works great.

Not sure what I was running at this point, as I found the project I had been using to test this, and tried just now but it failed in the VS2022 preview.

Managed to update the example to work, and instead use C++20s std::format instead.

#include <format>

   ... Same code
   
template <typename... Args>
void warn(format_with_location fmt, Args &&...args)
{
    spdlog::default_logger_raw( )-> log(fmt.loc, spdlog::level::warn, std::vformat(fmt.value, std::make_format_args(std::forward<Args>(args)...)));
}   

@gabime
Copy link
Owner

gabime commented Apr 26, 2024

Implemented in v2.x

@gabime gabime closed this as completed Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants