eddelbuettel
Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag
When building with SPDLOG_USE_STD_FORMAT spdlog hides the fmt dependency but rcppspdlog still tries to access the fmt library. This PR provides alternative implementation for these functions using the <format> library. Applicable only for C++20 and onwards.
The intent here is to permit compilation under C++20 with an option to support then-included fmt. That is good. But we can test that locally:
modified src/Makevars
@@ -1,3 +1,5 @@
+CXX_STD = CXX20
+PKG_CXXFLAGS = -DSPDLOG_USE_STD_FORMAT
## We need the spdlog headers
PKG_CPPFLAGS = -I../inst/include/
When I do that, I get both C++20 and the new define. And then the build fails:
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0}; std::format_args = std:
:basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0>]’:
formatter.cpp:33:25: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 1; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
formatter.cpp:40:74: required from here
formatter.cpp:28:33: warning: returning reference to temporary [-Wreturn-local-addr]
28 | return std::make_format_args(vec[S]...);
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0, 1}; std::format_args = s
td::basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0, 1>]’:
formatter.cpp:33:25: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 2; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
formatter.cpp:41:74: required from here
formatter.cpp:28:33: warning: returning reference to temporary [-Wreturn-local-addr]
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0, 1, 2}; std::format_args
= std::basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0, 1, 2>]’:
formatter.cpp:33:25: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 3; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
formatter.cpp:42:74: required from here
formatter.cpp:28:33: warning: returning reference to temporary [-Wreturn-local-addr]
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0, 1, 2, 3}; std::format_ar
gs = std::basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0, 1, 2, 3>]’:
formatter.cpp:33:25: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 4; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
formatter.cpp:43:74: required from here
formatter.cpp:28:33: warning: returning reference to temporary [-Wreturn-local-addr]
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0, 1, 2, 3, 4}; std::format
_args = std::basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0, 1, 2, 3, 4>]’:
formatter.cpp:33:25: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 5; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
formatter.cpp:44:74: required from here
formatter.cpp:28:33: warning: returning reference to temporary [-Wreturn-local-addr]
[.... many more lines omitted ...]
It also fails to compile RcppExports.cpp, a generated file (using Rcpp::compileAttributes()). I tried g++-13, g++-14, clang++-17 and clang++-18. All failed.
The intent here is to permit compilation under C++20 with an option to support then-included fmt. That is good. But we can test that locally
Disregard that. The CRAN version also fails under C++20 so that is a TODO item for me. We could still need a (simple) tester package though.
I think there may be two distinct PRs here. Both are welcome. One is the change to src/formatter.cpp which we could also condition on C++20 or greater and which allows compilation. When I add that to the main branch (and request C++20 compilation via src/Makevars) it installs under C++20 but leaves a bunch of not-so-nice-looking warnings behind.
edd@rob:~/git/rcppspdlog(master)$ R CMD INSTALL .
* installing to library ‘/usr/local/lib/R/site-library’
* installing *source* package ‘RcppSpdlog’ ...
** using staged installation
** libs
using C++ compiler: ‘g++-14 (Ubuntu 14-20240412-0ubuntu1) 14.0.1 20240412 (experimental) [master r14-9935-g67e1433a94f]’ using C++20
ccache g++-14 -std=gnu++20 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -I'/usr/local/lib/R/site-library/Rcpp/include' -fpic -O3 -Wall -pipe -pedantic -fdiagnostics-color=always -Wformat -c Rcpp
Exports.cpp -o RcppExports.o
ccache g++-14 -std=gnu++20 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -I'/usr/local/lib/R/site-library/Rcpp/include' -fpic -O3 -Wall -pipe -pedantic -fdiagnostics-color=always -Wformat -c exam
pleRsink.cpp -o exampleRsink.o
ccache g++-14 -std=gnu++20 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -I'/usr/local/lib/R/site-library/Rcpp/include' -fpic -O3 -Wall -pipe -pedantic -fdiagnostics-color=always -Wformat -c form
atter.cpp -o formatter.o
ccache g++-14 -std=gnu++20 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -I'/usr/local/lib/R/site-library/Rcpp/include' -fpic -O3 -Wall -pipe -pedantic -fdiagnostics-color=always -Wformat -c inte
rface.cpp -o interface.o
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0}; std::format_args = std:
:basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0>]’:
formatter.cpp:14:29: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 1; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
14 | return unpack_vector(vec, std::make_index_sequence<size>());
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
formatter.cpp:63:74: required from here
63 | case 1: return std::vformat(std::string_view(s), unpack_vector<1>(v));
| ~~~~~~~~~~~~~~~~^~~
formatter.cpp:9:37: warning: returning reference to temporary [-Wreturn-local-addr]
9 | return std::make_format_args(vec[S]...);
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
formatter.cpp: In instantiation of ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&, std::index_sequence<S ...>) [with long unsigned int ...S = {0, 1}; std::format_args = s
td::basic_format_args<std::basic_format_context<std::__format::_Sink_iter<char>, char> >; std::index_sequence<S ...> = std::integer_sequence<long unsigned int, 0, 1>]’:
formatter.cpp:14:29: required from ‘std::format_args&& unpack_vector(const std::vector<std::__cxx11::basic_string<char> >&) [with long unsigned int size = 2; std::format_args = std::basic_format_args<std::basi
c_format_context<std::__format::_Sink_iter<char>, char> >]’
formatter.cpp:9:37: warning: 14 | return unpack_vector(vec, std::make_index_sequence<size>());
formatter.cpp:9:37: warning: | ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
formatter.cpp:64:74: required from here
formatter.cpp:9:37: warning: 64 | case 2: return std::vformat(std::string_view(s), unpack_vector<2>(v));
formatter.cpp:9:37: warning: | ~~~~~~~~~~~~~~~~^~~
formatter.cpp:9:37: warning: returning reference to temporary [-Wreturn-local-addr]
9 | return std::make_format_args(vec[S]...);
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
... additional similar lines omitted ...
It seems to me we should fix this and have it compile without references to temporaries. Could you take a look?
The other part is optional switch to std::format instead of fmt::format. Despite the PR title, there is no added dependency here as spdlog (and RcppSpdlog) ship their vendored copy. But we could potentially offer to skip instantiation of fmt::format.
There may also be a conceptual "gotcha" here. We are really considering two things:
- How is the
RcppSpdlogpackage installed on the system? For this step we do not need C++20 to use C++20 later - How can a client package use the headers provided by
RcppSpdlog, and can it use C++20?
The answer to the second (and likely more relevant) question is 'yes'. To prove this, I took the three example source files (that are quick to be Rcpp::sourceCpp()-ed, no need to create a package) and added these two lines at the top prior to the include to spdlog:
#define SPDLOG_USE_STD_FORMAT
// [[Rcpp::plugins(cpp20)]]
They, respectively, define the C++20 option of not using fmt, and tell Rcpp to turn C++20 on. The first and third, doing vanilla RcppSpdlog things, work as is. The second does much more not usually exposed things, and runs into an error. I have not looked closely. But below is a the full example of the first, in verbose mode showing C++20 compilation.
> Rcpp::sourceCpp("/tmp/r/rcppspdlog/examples/exampleOne.cpp", verbose=TRUE, rebuild=TRUE)
Generated extern "C" functions
--------------------------------------------------------
#include <Rcpp.h>
#ifdef RCPP_USE_GLOBAL_ROSTREAM
Rcpp::Rostream<true>& Rcpp::Rcout = Rcpp::Rcpp_cout_get();
Rcpp::Rostream<false>& Rcpp::Rcerr = Rcpp::Rcpp_cerr_get();
#endif
// exampleOne
void exampleOne();
RcppExport SEXP sourceCpp_1_exampleOne() {
BEGIN_RCPP
Rcpp::RNGScope rcpp_rngScope_gen;
exampleOne();
return R_NilValue;
END_RCPP
}
Generated R functions
-------------------------------------------------------
`.sourceCpp_1_DLLInfo` <- dyn.load('/tmp/r/RtmpTy8XQi/sourceCpp-x86_64-pc-linux-gnu-1.0.13.3/sourcecpp_6891b17c343c9/sourceCpp_8.so')
exampleOne <- Rcpp:::sourceCppFunction(function() {}, TRUE, `.sourceCpp_1_DLLInfo`, 'sourceCpp_1_exampleOne')
rm(`.sourceCpp_1_DLLInfo`)
Building shared library
--------------------------------------------------------
DIR: /tmp/r/RtmpTy8XQi/sourceCpp-x86_64-pc-linux-gnu-1.0.13.3/sourcecpp_6891b17c343c9
/usr/lib/R/bin/R CMD SHLIB --preclean -o 'sourceCpp_8.so' 'exampleOne.cpp'
using C++ compiler: ‘g++-14 (Ubuntu 14-20240412-0ubuntu1) 14.0.1 20240412 (experimental) [master r14-9935-g67e1433a94f]’
using C++20
ccache g++-14 -std=gnu++20 -I"/usr/share/R/include" -DNDEBUG -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/usr/local/lib/R/site-library/RcppSpdlog/include" -I"/tmp/r/rcppspdlog/examples" -fpic -O3 -Wall -pipe -pedantic -fdiagnostics-color=always -Wformat -c exampleOne.cpp -o exampleOne.o
ccache g++-14 -std=gnu++20 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -Wl,-z,relro -o sourceCpp_8.so exampleOne.o -L/usr/lib/R/lib -lR
> exampleOne()
[19:30:31.742991] [I] [thread 428315] Welcome to spdlog!
[19:30:31.743001] [E] [thread 428315] Some error message with arg: 1
[19:30:31.743004] [W] [thread 428315] Easy padding in numbers like 00000012
[19:30:31.743008] [C] [thread 428315] Support for int: 42; hex: 2a; oct: 52; bin: 101010
[19:30:31.743012] [I] [thread 428315] Support for floats 1.23
[19:30:31.743015] [I] [thread 428315] Positional args are supported too..
[19:30:31.743018] [I] [thread 428315] left aligned
[19:30:31.743020] [D] [thread 428315] This message should be displayed..
>
</details>
PS The above may still instantiate fmt too.
I took a quick look at the 'returning reference to temporary' issue. It goes away if we return by value.
modified src/formatter.cpp
@@ -24,12 +24,12 @@
#ifdef SPDLOG_USE_STD_FORMAT
template<std::size_t... S>
-std::format_args&& unpack_vector(const std::vector<std::string>& vec, std::index_sequence<S...>) {
+std::format_args unpack_vector(const std::vector<std::string>& vec, std::index_sequence<S...>) {
return std::make_format_args(vec[S]...);
}
template<std::size_t size>
-std::format_args&& unpack_vector(const std::vector<std::string>& vec) {
+std::format_args unpack_vector(const std::vector<std::string>& vec) {
return unpack_vector(vec, std::make_index_sequence<size>());
}
The failure in the second example is originating from fmt and occurs to me with C++17, C++20 and C++20 with SPDLOG_USE_STD_FORMAT.
Extend spdlog for custom type
Last version is simpler which is better. One question above.
We folded another PR since this started so a rebase would be good, and an addition to ChangeLog (see format notes in PR 20 discussion, it is pretty simple) would be appreciated as a quick one paragraph summary for the three changed files (we can ignore .gitignore methinks).
The failure in the second example is originating from
fmt
I agree. It is trying to do more, all of which worked when I set it up years under (likely) C++11.
Could you try a rebase instead so that we get to a more linear history, please?
Thanks for doing the rebase.
It would still be nice to
- see a ChangeLog
- see about erroring if the define is set without C++20 active
- namespace refinement instead of C macro (but I could do that later too)
I may have to revert this. Upstream spdlog 1.15.0 just released, and applied to RcppSpdlog with this PR creates compilation errors in spdl. Applying only the spdlog 1.15.0 update to the CRAN release of RcppSpdlog works which points fingers at these changes here.
It is more important to keep up to date with spdlog than to support optional compilation without fmt.
Ok, I may reopen it once I have it working with the new spdlog version.
I made a really simple change to src/formatter.cpp basically just #if / #elif wrappen what we had before in one case, and what you added in the other keeping them separate. That will work as it does on the branch I have here.
Pushed and merged now as part of #21. If you want to replicate / see what it does, the actual error was that under the PR 'as is' the dependent package spdl no longer compiled but segfaulted unwinding the argument vector. This now works. Switching to variable argument length unrollment / template trick is actually independent of what we do with fmt::format or std::format so could be addressed another day or way.