Re: r338641 - [AST][4/4] Move the bit-fields from ObjCMethodDecl and ObjCContainerDecl into DeclContext
FYI, this breaks clang:
.../llvm/tools/clang/lib/AST/DeclObjC.cpp:793:7: error: static_assert
expression is not an integral constant expression
static_cast(ObjCMethodDeclBits.ObjCMethodFamilyBitWidth) ==
^
.../llvm/tools/clang/lib/AST/DeclObjC.cpp:793:29: note: implicit use of
'this' pointer is only allowed within the evaluation of a call to a
'constexpr' member function
static_cast(ObjCMethodDeclBits.ObjCMethodFamilyBitWidth) ==
^
1 error generated.
On Wed, Aug 1, 2018 at 2:31 PM Erich Keane via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> Author: erichkeane
> Date: Wed Aug 1 14:31:08 2018
> New Revision: 338641
>
> URL: http://llvm.org/viewvc/llvm-project?rev=338641&view=rev
> Log:
> [AST][4/4] Move the bit-fields from ObjCMethodDecl and ObjCContainerDecl
> into DeclContext
>
> This patch follows https://reviews.llvm.org/D49729,
> https://reviews.llvm.org/D49732 and
> https://reviews.llvm.org/D49733.
>
> Move the bits from ObjCMethodDecl and ObjCContainerDecl
> into DeclContext.
>
> Differential Revision: https://reviews.llvm.org/D49734
>
> Patch By: bricci
>
> Modified:
> cfe/trunk/include/clang/AST/DeclObjC.h
> cfe/trunk/lib/AST/DeclObjC.cpp
> cfe/trunk/lib/Sema/SemaDeclObjC.cpp
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
>
> Modified: cfe/trunk/include/clang/AST/DeclObjC.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=338641&r1=338640&r2=338641&view=diff
>
> ==
> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
> +++ cfe/trunk/include/clang/AST/DeclObjC.h Wed Aug 1 14:31:08 2018
> @@ -141,58 +141,10 @@ public:
>enum ImplementationControl { None, Required, Optional };
>
> private:
> - // The conventional meaning of this method; an ObjCMethodFamily.
> - // This is not serialized; instead, it is computed on demand and
> - // cached.
> - mutable unsigned Family : ObjCMethodFamilyBitWidth;
> -
> - /// instance (true) or class (false) method.
> - unsigned IsInstance : 1;
> - unsigned IsVariadic : 1;
> -
> - /// True if this method is the getter or setter for an explicit
> property.
> - unsigned IsPropertyAccessor : 1;
> -
> - // Method has a definition.
> - unsigned IsDefined : 1;
> -
> - /// Method redeclaration in the same interface.
> - unsigned IsRedeclaration : 1;
> -
> - /// Is redeclared in the same interface.
> - mutable unsigned HasRedeclaration : 1;
> -
> - // NOTE: VC++ treats enums as signed, avoid using ImplementationControl
> enum
> - /// \@required/\@optional
> - unsigned DeclImplementation : 2;
> -
> - // NOTE: VC++ treats enums as signed, avoid using the ObjCDeclQualifier
> enum
> - /// in, inout, etc.
> - unsigned objcDeclQualifier : 7;
> -
> - /// Indicates whether this method has a related result type.
> - unsigned RelatedResultType : 1;
> -
> - /// Whether the locations of the selector identifiers are in a
> - /// "standard" position, a enum SelectorLocationsKind.
> - unsigned SelLocsKind : 2;
> -
> - /// Whether this method overrides any other in the class hierarchy.
> - ///
> - /// A method is said to override any method in the class's
> - /// base classes, its protocols, or its categories' protocols, that has
> - /// the same selector and is of the same kind (class or instance).
> - /// A method in an implementation is not considered as overriding the
> same
> - /// method in the interface or its categories.
> - unsigned IsOverriding : 1;
> -
> - /// Indicates if the method was a definition but its body was skipped.
> - unsigned HasSkippedBody : 1;
> -
> - // Return type of this method.
> + /// Return type of this method.
>QualType MethodDeclType;
>
> - // Type source information for the return type.
> + /// Type source information for the return type.
>TypeSourceInfo *ReturnTInfo;
>
>/// Array of ParmVarDecls for the formal parameters of this method
> @@ -203,7 +155,7 @@ private:
>/// List of attributes for this method declaration.
>SourceLocation DeclEndLoc; // the location of the ';' or '{'.
>
> - // The following are only used for method definitions, null otherwise.
> + /// The following are only used for method definitions, null otherwise.
>LazyDeclStmtPtr Body;
>
>/// SelfDecl - Decl for the implicit self parameter. This is lazily
> @@ -220,21 +172,14 @@ private:
> bool isVariadic = false, bool isPropertyAccessor = false,
> bool isImplicitlyDeclared = false, bool isDefined =
> false,
> ImplementationControl impControl = None,
> - bool HasRelatedResultType = false)
> - : NamedDecl(ObjCMethod, contextDecl, beginLoc, SelInfo),
> -DeclContext(ObjCMethod), Family(InvalidObjCMethodFamily),
> -IsInstance(is
Re: r338641 - [AST][4/4] Move the bit-fields from ObjCMethodDecl and ObjCContainerDecl into DeclContext
(And it was fixed in r338648)
On Wed, Aug 1, 2018 at 3:24 PM David Jones wrote:
> FYI, this breaks clang:
>
> .../llvm/tools/clang/lib/AST/DeclObjC.cpp:793:7: error: static_assert
> expression is not an integral constant expression
> static_cast(ObjCMethodDeclBits.ObjCMethodFamilyBitWidth) ==
> ^
> .../llvm/tools/clang/lib/AST/DeclObjC.cpp:793:29: note: implicit use of
> 'this' pointer is only allowed within the evaluation of a call to a
> 'constexpr' member function
> static_cast(ObjCMethodDeclBits.ObjCMethodFamilyBitWidth) ==
> ^
> 1 error generated.
>
>
> On Wed, Aug 1, 2018 at 2:31 PM Erich Keane via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: erichkeane
>> Date: Wed Aug 1 14:31:08 2018
>> New Revision: 338641
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=338641&view=rev
>> Log:
>> [AST][4/4] Move the bit-fields from ObjCMethodDecl and ObjCContainerDecl
>> into DeclContext
>>
>> This patch follows https://reviews.llvm.org/D49729,
>> https://reviews.llvm.org/D49732 and
>> https://reviews.llvm.org/D49733.
>>
>> Move the bits from ObjCMethodDecl and ObjCContainerDecl
>> into DeclContext.
>>
>> Differential Revision: https://reviews.llvm.org/D49734
>>
>> Patch By: bricci
>>
>> Modified:
>> cfe/trunk/include/clang/AST/DeclObjC.h
>> cfe/trunk/lib/AST/DeclObjC.cpp
>> cfe/trunk/lib/Sema/SemaDeclObjC.cpp
>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>> cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/DeclObjC.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=338641&r1=338640&r2=338641&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclObjC.h Wed Aug 1 14:31:08 2018
>> @@ -141,58 +141,10 @@ public:
>>enum ImplementationControl { None, Required, Optional };
>>
>> private:
>> - // The conventional meaning of this method; an ObjCMethodFamily.
>> - // This is not serialized; instead, it is computed on demand and
>> - // cached.
>> - mutable unsigned Family : ObjCMethodFamilyBitWidth;
>> -
>> - /// instance (true) or class (false) method.
>> - unsigned IsInstance : 1;
>> - unsigned IsVariadic : 1;
>> -
>> - /// True if this method is the getter or setter for an explicit
>> property.
>> - unsigned IsPropertyAccessor : 1;
>> -
>> - // Method has a definition.
>> - unsigned IsDefined : 1;
>> -
>> - /// Method redeclaration in the same interface.
>> - unsigned IsRedeclaration : 1;
>> -
>> - /// Is redeclared in the same interface.
>> - mutable unsigned HasRedeclaration : 1;
>> -
>> - // NOTE: VC++ treats enums as signed, avoid using
>> ImplementationControl enum
>> - /// \@required/\@optional
>> - unsigned DeclImplementation : 2;
>> -
>> - // NOTE: VC++ treats enums as signed, avoid using the
>> ObjCDeclQualifier enum
>> - /// in, inout, etc.
>> - unsigned objcDeclQualifier : 7;
>> -
>> - /// Indicates whether this method has a related result type.
>> - unsigned RelatedResultType : 1;
>> -
>> - /// Whether the locations of the selector identifiers are in a
>> - /// "standard" position, a enum SelectorLocationsKind.
>> - unsigned SelLocsKind : 2;
>> -
>> - /// Whether this method overrides any other in the class hierarchy.
>> - ///
>> - /// A method is said to override any method in the class's
>> - /// base classes, its protocols, or its categories' protocols, that has
>> - /// the same selector and is of the same kind (class or instance).
>> - /// A method in an implementation is not considered as overriding the
>> same
>> - /// method in the interface or its categories.
>> - unsigned IsOverriding : 1;
>> -
>> - /// Indicates if the method was a definition but its body was skipped.
>> - unsigned HasSkippedBody : 1;
>> -
>> - // Return type of this method.
>> + /// Return type of this method.
>>QualType MethodDeclType;
>>
>> - // Type source information for the return type.
>> + /// Type source information for the return type.
>>TypeSourceInfo *ReturnTInfo;
>>
>>/// Array of ParmVarDecls for the formal parameters of this method
>> @@ -203,7 +155,7 @@ private:
>>/// List of attributes for this method declaration.
>>SourceLocation DeclEndLoc; // the location of the ';' or '{'.
>>
>> - // The following are only used for method definitions, null otherwise.
>> + /// The following are only used for method definitions, null otherwise.
>>LazyDeclStmtPtr Body;
>>
>>/// SelfDecl - Decl for the implicit self parameter. This is lazily
>> @@ -220,21 +172,14 @@ private:
>> bool isVariadic = false, bool isPropertyAccessor =
>> false,
>> bool isImplicitlyDeclared = false, bool isDefined =
>> false,
>> ImplementationControl impC
Re: r335084 - Append new attributes to the end of an AttributeList.
(Sorry for the late reply...)
On Mon, Jun 25, 2018 at 2:45 PM Michael Kruse via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> I just revert if to have further discussions (r335516)
>
> Michael
>
> 2018-06-25 14:58 GMT-05:00 Eric Christopher :
> >
> >
> > On Mon, Jun 25, 2018 at 12:21 PM Richard Smith via cfe-commits
> > wrote:
> >>
> >> On 23 June 2018 at 22:34, Michael Kruse via cfe-commits
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> multiple comments in the code indicate that the attribute order was
> >>> surprising and probably has lead to bugs, and will lead to bugs in the
> >>> future. The order had to be explicitly reversed to avoid those. This
> >>> alone for me this seems a good reason to have the attributes in the
> >>> order in which they appear in the source.
> >>>
> >>> It would be nice it you could send a reproducer. At a glance, your
> >>> case does not seem related since the format strings are function
> >>> arguments, not attributes.
>
Well, there is a nested function call, but the format warning applies to
two different parameters: a singular phrasing string, and a plural one. A
reproducer would look like this, which is almost verbatim from the link:
$ cat /tmp/repro.cc
#include
#include
void foo(){ printf(ngettext("one frobber", "%lu frobbers", 0UL), 0UL); }
$ /bin/clang -Wall -c -o /dev/null /tmp/repro.cc
/tmp/repro.cc:4:66: warning: data argument not used by format string
[-Wformat-extra-args]
void foo(){ printf(ngettext("one frobber", "%lu frobbers", 0UL), 0UL); }
~^
1 warning generated.
Swapping the string params works, and an older revision yields inverse
results.
> >>> 2018-06-23 17:17 GMT-05:00 David Jones :
> >>> > Since the semantics of the ordering of multiple format args seems
> >>> > somewhat
> >>> > ill-defined, it seems to me like reverting may be the best near-term
> >>> > choice.
> >>> > I can imagine other ways to fix the diagnostic, but the only
> behaviour
> >>> > that
> >>> > I would believe to be expected by existing code is the old one, so a
> >>> > change
> >>> > like this one probably needs to be more carefully vetted before being
> >>> > (re-)committed.
> >>>
> >>> Could you give more details about your concerns?
>
Multiple format strings (or repeating or reordering other attributes) seems
like it lacks clear semantics. I can't find any particularly
well-documented explanation of Clang's behaviour in these cases, which
makes me suspect that most people would just "fix it until it works" and
move on. GCC's documentation doesn't seem to be very decisive, either.
For example, you mentioned that multiple format attributes may be invalid;
if so, then rejecting such cases should probably be a part of the fix. But
that would require some vetting, since this (pretty clearly) doesn't mirror
the current reality.
>> (I'm not sure what the problem is, but as a data point, Sema::checkCall
> >> iterates over the FormatAttrs in order, so it's possible that changing
> the
> >> order may have triggered a new warning. That may be due to a
> pre-existing
> >> order-dependence bug, or it may be that we're losing an attribute here.)
>
It's arguably a pre-existing order dependence bug in ngettext, but it would
be ... well, a fairly prevalent bug, since it could churn any such calls
through libintl/gettext/etc.
> >>> > Please let me know your plan. (If I don't hear back in a day or so,
> >>> > I'll go
> >>> > ahead and revert for you as the safe default course of action.)
> >>>
> >>> On a weekend?
> >>
> >>
> >> Yes; our policy is generally to revert to green if a patch causes
> >> regressions that aren't going to be fixed in a few hours. This is
> generally
> >> good for the committer, because it lets you figure out what's wrong and
> deal
> >> with it on your own schedule rather than being pressured to fix it
> urgently
> >> because you're blocking the work of others.
Well, the bug did repro on a weekend, so there's that. ;-)
My main goal was to give a tighter response timeout to account for any
timezone drift (with a target of Monday ~everywhere).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
