approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
310 stars 50 forks source link

Bug with {fmt} integration #213

Open YarikTH opened 9 months ago

YarikTH commented 9 months ago

Integration with {fmt} has a problem if only <fmt/core.h> is included. Because it defines FMT_VERSION that triggers {fmt} integration, but does not provides fmt::to_string which is defined in <fmt/format.h>

https://godbolt.org/z/3WTE6f3b6

<source>: In static member function 'static std::string ApprovalTests::FmtToString::toString(const T&)':
<source>:1028:25: error: 'to_string' is not a member of 'fmt'; did you mean 'std::__cxx11::to_string'?
 1028 |             return fmt::to_string(printable);
      |                         ^~~~~~~~~
In file included from /opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/string:54,
                 from /opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/bits/locale_classes.h:40,
                 from /opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/bits/ios_base.h:41,
                 from /opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/streambuf:43,
                 from /opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/bits/streambuf_iterator.h:35,
                 from /opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/iterator:66,
                 from /opt/compiler-explorer/libs/fmt/10.1.1/include/fmt/core.h:14,
                 from <source>:1:
/opt/compiler-explorer/gcc-13.2.0/include/c++/13.2.0/bits/basic_string.h:4249:3: note: 'std::__cxx11::to_string' declared here
 4249 |   to_string(long double __val)
      |   ^~~~~~~~~
Compiler returned: 1
YarikTH commented 9 months ago

https://github.com/approvals/ApprovalTests.cpp/issues/124

YarikTH commented 9 months ago

Possible solutions:

  1. Include <fmt/format.h> if FMT_VERSION is detected
  2. Redo fmt detection to use FMT_FORMATH define guard (looks unstable)
  3. Include <fmt/format.h> and fmt integration manually instead of autodetect. For e.g. #ifdef APPROVALS_USE_FMT. At least integration with other testing frameworks is done this way instead of using hacky detections

1st looks more stable for v10, while I would prefer 3rd option as a v11 solution. I personally bumped up in this bug, when I included header that includes <fmt/core.h> in the approvals based test code. It is not the best way to bump into library feature