-
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
Misc warnings #2801
Misc warnings #2801
Conversation
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/compile.h
Outdated
@@ -36,7 +36,7 @@ template <typename OutputIt> class truncating_iterator_base { | |||
using difference_type = std::ptrdiff_t; | |||
using pointer = void; | |||
using reference = void; | |||
using _Unchecked_type = | |||
using Unchecked_type_ = |
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.
This will introduce warnings in MSVC, please revert.
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 warning would it introduce?
Underscore followed by uppercase is reserved in C++: https://en.cppreference.com/w/cpp/language/identifiers
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.
Warnings about (un)checked iterators.
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.
Is _Unchecked_type
some kind of Microsoft extension keyword?
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.
Sort of. It's not a keyword but some mechanism to opt out iterators.
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 we somewhere do something like:
#if defined(_MSC_VER)
#define fmt_unchecked_type _Unchecked_type
#else
#define fmt_unchecked_type
#endif
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.
If you have access to MSVC 2015, I'd recommend checking if removing _Unchecked_type
altogether still produces warnings. It might be no longer the case as IIRC they have been moving away from the checked iterators. You could even submit a test PR to the fmt repo and check warnings in appveyor: https://ci.appveyor.com/project/vitaut/fmt.
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.
Actually, we can see on this PR that there are definitely more warnings: https://ci.appveyor.com/project/vitaut/fmt/builds/42816914/job/briwktqjiiw9oa34. The macro approach seems OK but note that macros should be uppercase.
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.
OK, see new attempt.
include/fmt/core.h
Outdated
@@ -1498,7 +1498,7 @@ class appender : public std::back_insert_iterator<detail::buffer<char>> { | |||
public: | |||
using std::back_insert_iterator<detail::buffer<char>>::back_insert_iterator; | |||
appender(base it) noexcept : base(it) {} | |||
using _Unchecked_type = appender; // Mark iterator as checked. | |||
using Unchecked_type_ = appender; // Mark iterator as checked. |
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.
same here
include/fmt/compile.h
Outdated
@@ -8,6 +8,7 @@ | |||
#ifndef FMT_COMPILE_H_ | |||
#define FMT_COMPILE_H_ | |||
|
|||
#include "core.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.
This is not needed: core.h is a lightweight alternative to format.h, it doesn't make sense to include it when format.h is included.
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.
Removed.
include/fmt/core.h
Outdated
#ifdef _MSC_VER | ||
# define FMT_UNCHECKED_TYPE _Unchecked_type | ||
#else | ||
# define FMT_UNCHECKED_TYPE DummyName | ||
#endif |
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.
Let's change it into:
#define FMT_UNCHECKED_ITERATOR(It) using _Unchecked_type = It; // Mark iterator as checked.
to simplify call sites.
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.
include/fmt/core.h
Outdated
@@ -2002,11 +2008,11 @@ using format_args = basic_format_args<format_context>; | |||
// We cannot use enum classes as bit fields because of a gcc bug | |||
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414. | |||
namespace align { | |||
enum type { none, left, right, center, numeric }; | |||
enum type : unsigned { none, left, right, center, numeric }; |
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 unsigned?
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.
As per commit message: to fix the -Wsigned-enum-bitfield warnings.
include/fmt/compile.h
Outdated
using FMT_UNCHECKED_TYPE = | ||
truncating_iterator_base; // Mark iterator as checked. |
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.
This should be `FMT_UNCHECKED_TYPE(truncating_iterator_base);
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.
Doh, sorry I missed that one. Fixed.
include/fmt/core.h
Outdated
# define FMT_UNCHECKED_ITERATOR(It) using _Unchecked_type = It; // Mark iterator as checked. | ||
#else | ||
# define FMT_UNCHECKED_ITERATOR(It) using DummyTypeName = It; |
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.
Let's remove trailing semicolons since those are added at call sites.
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.
Ah, good point. Fixed.
Please fix build failures. |
And apply clang-format. |
I don't understand that error. I played on godbolt, and it seems to me like a gcc bug fixed between 9.2 and 9.3: https://godbolt.org/z/58aEv8zEq Interesting, the warning goes away if I change the enum from |
If that resolves the issue then sure. |
There are still build failures. |
Do you still plan working on this PR? |
Yes. But I'm at a loss as to why that old gcc complains. I don't have a means to reproduce it locally. I guess I could split that one commit out so the rest can be merged... |
Avoid defining various reserved identifiers (starting with underscore and capital letter). Fortunately, they were all Windows-only, so it was easy to conditionalize them in Window-only preprocessor checks.
Created FMT_UNCHECKED_TYPE that resolves to special identifier _Unchecked_type for Microsoft, but to a dummy string otherwise. Using _Unchecked_type is invalid because underscore + uppercase is a reserved identifier.
Thanks |
No description provided.