Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag #19

XanthosXanthopoulos · 2024-10-11T10:54:51Z

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.

inst/include/rcpp_sink.h

.gitignore

eddelbuettel · 2024-10-11T13:10:02Z

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.

eddelbuettel · 2024-10-11T13:20:40Z

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.

src/formatter.cpp

eddelbuettel · 2024-10-11T16:22:33Z

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.

eddelbuettel · 2024-10-12T00:31:45Z

There may also be a conceptual "gotcha" here. We are really considering two things:

How is the RcppSpdlog package 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.

eddelbuettel · 2024-10-12T12:51:55Z

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>());
 }

eddelbuettel · 2024-10-14T13:10:18Z

src/formatter.cpp

+#define NAMESPACE std
+#else
+#define NAMESPACE fmt
+#endif


One can do this with a typedef too.

The combination of the C++20 test and the *USE_STD_FORMAT only here is ... a little odd. Maybe we should check if *USE_STD_FORMAT is set and error if it is not C++20 as well? Or check in all other uses?

Yes erroring out if not C++20 makes more sense

XanthosXanthopoulos · 2024-10-14T13:11:48Z

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

eddelbuettel · 2024-10-14T13:12:30Z

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).

eddelbuettel · 2024-10-14T13:15:44Z

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.

eddelbuettel · 2024-10-14T14:22:31Z

Could you try a rebase instead so that we get to a more linear history, please?

eddelbuettel · 2024-10-15T01:40:24Z

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)

eddelbuettel

As commented a few days ago, it would still be nice to

see a ChangeLog
see about erroring if the define is set without C++20 active

Please add those.

…withput C++20 or greater

eddelbuettel · 2024-10-29T12:02:52Z

ChangeLog

@@ -1,3 +1,10 @@
+2024-10-29 Xanthos Xanthopoulos <[email protected]>


I uses two whitespaces here between date and name, and name and email, but I can touch that up. Same for line length.

eddelbuettel · 2024-10-29T12:03:00Z

ChangeLog

@@ -1,3 +1,10 @@
+2024-10-29 Xanthos Xanthopoulos <[email protected]>


I uses two whitespaces here between date and name, and name and email, but I can touch that up. Same for line length.

eddelbuettel · 2024-10-29T12:03:03Z

inst/include/RcppSpdlog_types.h

@@ -1,6 +1,10 @@

 #pragma once

+#if defined(SPDLOG_USE_STD_FORMAT) && __cplusplus < 202002L
+#error SPDLOG_USE_STD_FORMAT should only be set when compiling with C++20 and greater


Thanks. I may replace that 'and' with 'or'.

eddelbuettel

Minor comments inline.

eddelbuettel · 2024-11-09T15:31:01Z

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.

XanthosXanthopoulos · 2024-11-09T17:00:47Z

Ok, I may reopen it once I have it working with the new spdlog version.

eddelbuettel · 2024-11-09T18:24:51Z

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.

eddelbuettel · 2024-11-10T15:02:56Z

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.

eddelbuettel reviewed Oct 11, 2024

View reviewed changes

inst/include/rcpp_sink.h Show resolved Hide resolved

eddelbuettel reviewed Oct 11, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

eddelbuettel reviewed Oct 11, 2024

View reviewed changes

src/formatter.cpp Outdated Show resolved Hide resolved

eddelbuettel reviewed Oct 14, 2024

View reviewed changes

XanthosXanthopoulos added 3 commits October 14, 2024 17:46

Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag

58bc0e3

Fix Rccp convention break

fca75a7

Replace format with vformat and conditionally choose namespace

be7cf0a

XanthosXanthopoulos force-pushed the master branch from 8e3954a to be7cf0a Compare October 14, 2024 14:46

eddelbuettel requested changes Oct 25, 2024

View reviewed changes

XanthosXanthopoulos added 2 commits October 29, 2024 12:45

Error out with approperiate message when using SPDLOG_USE_STD_FORMAT …

defcc52

…withput C++20 or greater

Add changelog

e733b70

eddelbuettel reviewed Oct 29, 2024

View reviewed changes

eddelbuettel approved these changes Oct 29, 2024

View reviewed changes

eddelbuettel merged commit a5c648b into eddelbuettel:master Oct 29, 2024
1 check passed

eddelbuettel added a commit that referenced this pull request Oct 29, 2024

Roll micro release, and minor touchups on #19

892d95e

eddelbuettel added a commit that referenced this pull request Nov 10, 2024

Partially revert / simplify recent src/formatter.cpp changes in #19

1db1c21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag #19

Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag #19

XanthosXanthopoulos commented Oct 11, 2024

eddelbuettel commented Oct 11, 2024

eddelbuettel commented Oct 11, 2024

eddelbuettel commented Oct 11, 2024

eddelbuettel commented Oct 12, 2024 •

edited

Loading

eddelbuettel commented Oct 12, 2024

eddelbuettel Oct 14, 2024

XanthosXanthopoulos Oct 14, 2024

XanthosXanthopoulos commented Oct 14, 2024

eddelbuettel commented Oct 14, 2024

eddelbuettel commented Oct 14, 2024

eddelbuettel commented Oct 14, 2024

eddelbuettel commented Oct 15, 2024

eddelbuettel left a comment

eddelbuettel Oct 29, 2024

eddelbuettel Oct 29, 2024

eddelbuettel Oct 29, 2024

eddelbuettel left a comment

eddelbuettel commented Nov 9, 2024

XanthosXanthopoulos commented Nov 9, 2024

eddelbuettel commented Nov 9, 2024

eddelbuettel commented Nov 10, 2024

		@@ -1,3 +1,10 @@
		2024-10-29 Xanthos Xanthopoulos <[email protected]>

Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag #19

Remove fmt dependencies when build with SPDLOG_USE_STD_FORMAT flag #19

Conversation

XanthosXanthopoulos commented Oct 11, 2024

eddelbuettel commented Oct 11, 2024

eddelbuettel commented Oct 11, 2024

eddelbuettel commented Oct 11, 2024

eddelbuettel commented Oct 12, 2024 • edited Loading

eddelbuettel commented Oct 12, 2024

eddelbuettel Oct 14, 2024

Choose a reason for hiding this comment

XanthosXanthopoulos Oct 14, 2024

Choose a reason for hiding this comment

XanthosXanthopoulos commented Oct 14, 2024

eddelbuettel commented Oct 14, 2024

eddelbuettel commented Oct 14, 2024

eddelbuettel commented Oct 14, 2024

eddelbuettel commented Oct 15, 2024

eddelbuettel left a comment

Choose a reason for hiding this comment

eddelbuettel Oct 29, 2024

Choose a reason for hiding this comment

eddelbuettel Oct 29, 2024

Choose a reason for hiding this comment

eddelbuettel Oct 29, 2024

Choose a reason for hiding this comment

eddelbuettel left a comment

Choose a reason for hiding this comment

eddelbuettel commented Nov 9, 2024

XanthosXanthopoulos commented Nov 9, 2024

eddelbuettel commented Nov 9, 2024

eddelbuettel commented Nov 10, 2024

eddelbuettel commented Oct 12, 2024 •

edited

Loading