mehdi_amini reopened this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
Reopen: this hasn't been committed yet apparently.
@sashab : are you still interested in moving this forward?
https://reviews.llvm.org/D24289
sashab added a comment.
Sorry, had this discussion elsewhere
(https://bugs.chromium.org/p/chromium/issues/detail?id=648462).
I'm uncertain at this point. There is currently a bug in GCC that means enums
with an explicit underlying type (or enum classes, although the latter is to be
fixed) are
mehdi_amini added a comment.
> So when this modification tells the developer to add 'unsigned' to their
> enum, they are subsequently causing a warning to occur in GCC.
>
> I have commented on the bug on GCC for this
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242#c28), but it looks
> un
aaron.ballman added a comment.
In https://reviews.llvm.org/D24289#592592, @mehdi_amini wrote:
> > So when this modification tells the developer to add 'unsigned' to their
> > enum, they are subsequently causing a warning to occur in GCC.
> >
> > I have commented on the bug on GCC for this
> >
This revision was automatically updated to reflect the committed changes.
Closed by commit rL287177: Add warning when assigning enums to bitfields
without an explicit unsigned… (authored by rnk).
Changed prior to commit:
https://reviews.llvm.org/D24289?vs=70760&id=78281#toc
Repository:
rL LL
rsmith added a comment.
This is causing warnings to fire for headers shared between C and C++, where
the "give the enum an unsigned underlying type" advice doesn't work, and where
the code in question will never be built for the MS ABI. It seems really hard
to justify this being on by default.
mehdi_amini added a comment.
What is the correct solution for MSVC in C? (If any)
Repository:
rL LLVM
https://reviews.llvm.org/D24289
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
rnk added a comment.
In https://reviews.llvm.org/D24289#598169, @rsmith wrote:
> This is causing warnings to fire for headers shared between C and C++, where
> the "give the enum an unsigned underlying type" advice doesn't work, and
> where the code in question will never be built for the MS AB
rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.
lgtm
Comment at: lib/Sema/SemaChecking.cpp:7841
@@ +7840,3 @@
+if (S.getLangOpts().CPlusPlus11 &&
+!BitfieldEnumDecl->getIntegerTypeSourceInfo() &&
+BitfieldEnum
thakis added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2962
@@ -2959,1 +2961,3 @@
+ "the enum %0 an unsigned underlying type">,
+ InGroup>, DefaultIgnore;
def warn_attribute_packed_for_bitfield : Warning<
Hm, isn't the more safe de
aaron.ballman added a subscriber: aaron.ballman.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2962
@@ -2959,1 +2961,3 @@
+ "the enum %0 an unsigned underlying type">,
+ InGroup>, DefaultIgnore;
def warn_attribute_packed_for_bitfield : Warning<
thakis
rnk added a comment.
I was thinking of suggesting to put it under -Wextra, but -Wall is fine too.
I still don't think it should be on by default. I think there are a lot of
Unix-only projects out there that aren't concerned with MSVC compatibility, and
a warning about how things with in the Mic
sashab updated this revision to Diff 70760.
sashab marked an inline comment as done.
sashab added a comment.
Thanks for your feedback everyone; left the flag as DefaultIgnore but added it
to -Wall.
Keep in mind I am planning on adding two additional warnings after this, namely
"%0 is too small t
I'm a bit surprised that this landed, given that gcc bug. I can see the
motivation for the gcc bug: If you say your enum is going to need
underlying 8 bits, then warning that your bitfield where you store it is
smaller _is_ surprising.
I'm not sure if landing this while gcc still behaves the way i
On 16 November 2016 at 18:38, Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> I'm a bit surprised that this landed, given that gcc bug. I can see the
> motivation for the gcc bug: If you say your enum is going to need
> underlying 8 bits, then warning that your bitfield where you
On 17 Nov 2016 8:56 am, "Reid Kleckner" wrote:
rnk added a comment.
In https://reviews.llvm.org/D24289#598169, @rsmith wrote:
> This is causing warnings to fire for headers shared between C and C++,
where the "give the enum an unsigned underlying type" advice doesn't work,
and where the code in
On Thu, Nov 17, 2016 at 9:52 AM, Richard Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On 17 Nov 2016 8:56 am, "Reid Kleckner" wrote:
>> In https://reviews.llvm.org/D24289#598169, @rsmith wrote:
>>> This is causing warnings to fire for headers shared between C and C++,
>>> where the
Although I agree with your philosophical discussion and suggestions, the
reality is that MSVC's behavior is not a bug and compilers are free to
interpret enum bitfields with no explicit underlying type in any way they
want (see spec reference in GCC bug link), with a signed interpretation
being a v
On Thu, Nov 17, 2016 at 2:14 PM, Sasha Bermeister
wrote:
> Although I agree with your philosophical discussion and suggestions, the
> reality is that MSVC's behavior is not a bug and compilers are free to
> interpret enum bitfields with no explicit underlying type in any way they
> want (see spec
Thanks for the great explanation, I agree with this advice. I'm going to
investigate writing a new patch to do this.
On Fri, Nov 18, 2016 at 1:59 PM, Arthur O'Dwyer
wrote:
> On Thu, Nov 17, 2016 at 2:14 PM, Sasha Bermeister
> wrote:
>
>> Although I agree with your philosophical discussion and s
sashab added a comment.
Pinging for another LGTM since I have added it to -Wall and added a portability
comment to the error message :)
https://reviews.llvm.org/D24289
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cg
thakis added a comment.
Works for me if rnk likes it :-)
(We could bikeshed on if this should be one of the -Wmicrosoft warnings, but
the warning can't be both in -Wall and -Wmicrosoft, so let's don't).
I do have a question about the test (the first commit below):
Comment at:
rnk added a comment.
In https://reviews.llvm.org/D24289#543874, @thakis wrote:
> Works for me if rnk likes it :-)
Yep, looks good.
> (We could bikeshed on if this should be one of the -Wmicrosoft warnings, but
> the warning can't be both in -Wall and -Wmicrosoft, so let's don't).
I also don
aaron.ballman accepted this revision.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.
LGTM as well, thank you!
https://reviews.llvm.org/D24289
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.or
sashab added a comment.
Thanks all! :)
Comment at: test/SemaCXX/warn-msvc-enum-bitfield.cpp:12
@@ +11,3 @@
+
+ s.e2 = E2;
+ s.f2 = F2;
thakis wrote:
> Shouldn't this be the version that warns? The assignment with E1 assigns 0
> which is portable, but this ass
sashab marked an inline comment as done.
sashab added a comment.
https://reviews.llvm.org/D24289
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sashab closed this revision.
sashab added a comment.
Is this how I commit this? Hopefully this lands... :-)
https://reviews.llvm.org/D24289
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
thakis added inline comments.
Comment at: test/SemaCXX/warn-msvc-enum-bitfield.cpp:12
@@ +11,3 @@
+
+ s.e2 = E2;
+ s.f2 = F2;
sashab wrote:
> thakis wrote:
> > Shouldn't this be the version that warns? The assignment with E1 assigns 0
> > which is portable, but
28 matches
Mail list logo
