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

C++14 build fixes for older gcc #2333 #2372

Merged
merged 2 commits into from
May 13, 2022
Merged

C++14 build fixes for older gcc #2333 #2372

merged 2 commits into from
May 13, 2022

Conversation

kslattery
Copy link

@kslattery kslattery commented May 11, 2022

@gabime I'm unable to test that the changes for __has_include in bin_to_hex.h work for @aengusjiang, but the approach I've followed matches the recommended pattern in the gcc documentation for __has_include.

I have tested the rest of the changes on my machine with gcc version gcc (Ubuntu 5.5.0-12ubuntu1~14.04) 5.5.0 20171010. I get the overloaded formatter used as seen in the output from the example program:

[2022-05-11 18:22:25.008] [info] user defined type: [my_type i=14]

The problem seems to be caused by this clause in the namespace specification (emphasis added):

To reopen an existing namespace (formally, to be an extension-namespace-definition), the lookup for the identifier used in the namespace definition must resolve to a namespace name (not a namespace alias), that was declared as a member of the enclosing namespace or of an inline namespace within an enclosing namespace.

The approach I've taken here is the same approach used within fmt for the FMT_BEGIN_NAMESPACE macro defined in include/spdlog/fmt/bundled/core.h.

@gabime
Copy link
Owner

gabime commented May 12, 2022

Thanks a lot. I think the has_include fix is ok, but the namespace define can be replaced with an example instead. I prefer not to add yet another ifdef to the code if possible.

@kslattery
Copy link
Author

Makes sense. I wasn't sure what you'd want to do. The problem with just using namespace fmt in the example code is that the example will then not work if using the std formatter. I'd suggest putting something like this in the example to make it work either way:

#ifdef SPDLOG_USE_STD_FORMAT
    namespace std {
#else 
    namespace fmt {
#endif
template<>
struct formatter<my_type> : formatter<std::string>
...
}. // end of namespace

@gabime
Copy link
Owner

gabime commented May 12, 2022

I'd suggest putting something like this in the example to make it work either way

Yes. Thank you.

@kslattery
Copy link
Author

Ok, do you want me to create a new commit with those changes? Or do you want to just take it from here?

@gabime
Copy link
Owner

gabime commented May 12, 2022

Please do.

…namespace when fmt_lib namespace alias cannot be used.
@kslattery
Copy link
Author

@gabime If you don't like the comment I added, let me know what, if anything, you'd like in its place.

@gabime gabime merged commit 0ef5228 into gabime:v1.x May 13, 2022
@gabime
Copy link
Owner

gabime commented May 13, 2022

Thanks @kslattery

@anhns26693
Copy link

Thanks @kslattery very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants