-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add class name output to formatter for std::exception #3076
Conversation
b187daf
to
1d6ba76
Compare
edb2815
to
a47b280
Compare
Rebased and fixed bug related to GCC < 5. |
cc @Baardi, @zach2good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
include/fmt/core.h
Outdated
FMT_CONSTEXPR_CHAR_TRAITS bool starts_with( | ||
basic_string_view<Char> sv) const noexcept { | ||
return size_ >= sv.size_ && | ||
std::char_traits<Char>::compare(data_, sv.data_, sv.size_) == 0; | ||
} | ||
FMT_CONSTEXPR_CHAR_TRAITS bool starts_with(Char c) const noexcept { | ||
return size_ >= 1 && std::char_traits<Char>::eq(*data_, c); | ||
} | ||
FMT_CONSTEXPR_CHAR_TRAITS bool starts_with(const Char* s) const { | ||
return starts_with(basic_string_view<Char>(s)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to a separate PR and add unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #3080
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's a good point. |
@vitaut |
a47b280
to
d3c6804
Compare
I think we should keep the default format independent of ABI. Let's make type output an opt-in via a separate specifier (maybe |
Like range format specifications: |
d3c6804
to
93b7d39
Compare
@vitaut |
I like the idea. It makes sense to format the content of an item by default, which I guess would be the .what() on exceptions. Btw @phprus, with the current PR, it still formats the whole typeinfo.name(), even if it's not one of the "big 3" compilers. |
Please don't use |
@Baardi |
Good point. There's argument both ways. I guess that not specifying the formatter could lead to unexpected exceptions etc,, when trying to write cross-platform code, which would be a good argument for always speciyfing it. And I guess "important" platforms could always create PR's adding proper formatting, if they're not happy with the current way it is formatted. Anyways, regardless of whether this gets merged or not. |
93b7d39
to
98e342f
Compare
@vitaut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. A few comments/questions inline.
@@ -28,6 +31,16 @@ | |||
# endif | |||
#endif | |||
|
|||
#if FMT_HAS_INCLUDE(<cxxabi.h>) || defined(__GLIBCXX__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need __GLIBCXX__
check? Isn't FMT_HAS_INCLUDE
enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCC 4 does not support FMT_HAS_INCLUDE, but it does support demangling.
It is possible to check for the presence of libstdc++ instead of gcc version, since all libstdc++ support demangling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment that the __GLIBCXX__
is for gcc 4 here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -28,6 +31,16 @@ | |||
# endif | |||
#endif | |||
|
|||
#if FMT_HAS_INCLUDE(<cxxabi.h>) || defined(__GLIBCXX__) | |||
# include <cxxabi.h> | |||
# ifndef __GABIXX_CXXABI_H__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is __GABIXX_CXXABI_H__
check for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Android.
See comment from Boost:
// For some archtectures (mips, mips64, x86, x86_64) cxxabi.h in Android NDK is implemented by gabi++ library
// (https://android.googlesource.com/platform/ndk/+/master/sources/cxx-stl/gabi++/), which does not implement
// abi::__cxa_demangle(). We detect this implementation by checking the include guard here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
And I renamed FMT_HAS_CXXABI_H
to FMT_HAS_ABI_CXA_DEMANGLE
.
d30bf69
to
3ed02f7
Compare
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
3ed02f7
to
3019938
Compare
Thank you! |
Discussion:
#3062 (comment)