aaron.ballman added a comment.
Thank you for doing all this work -- in general, this is looking great and
really cleans things up!
================
Comment at: clang/include/clang/Basic/Attr.td:2266
let Spellings = [CXX11<"", "maybe_unused", 201603>, GCC<"unused">,
- C2x<"", "maybe_unused">];
+ C2x<"", "maybe_unused">, Pragma<"", "unused">];
let Subjects = SubjectList<[Var, ObjCIvar, Type, Enum, EnumConstant, Label,
----------------
I'm surprised to see this as part of an NFC refactoring. Hopefully it will
become more obvious later. :-D
================
Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:66-69
+ unsigned AttrKind : 16;
+ /// Corresponds to the Syntax enum.
+ unsigned SyntaxUsed : 3;
+ unsigned SpellingIndex : 4;
----------------
Do you want to do `= 0` here to give them in-class initializers? (well,
`SpellingIndex` should probably be `SpellingNotCalculated`).
================
Comment at: clang/include/clang/Lex/Preprocessor.h:373
- /// The source location of the currently-active
+ /// The source location and identifier of the currently-active
/// \#pragma clang arc_cf_code_audited begin.
----------------
Can you correct the order in the comment to be the same as the order in the
pair?
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:298-300
+ AttributeCommonInfo({}, AttributeCommonInfo::AT_Aligned,
+ AttributeCommonInfo::AS_GNU,
+ AlignedAttr::GNU_aligned)));
----------------
I'm surprised we'd give a parse syntax to an implicit attribute. Wouldn't we
rather note that these have no associated syntax or spelling?
================
Comment at: clang/lib/Sema/SemaAttr.cpp:892
= (VisibilityAttr::VisibilityType) rawType;
+
SourceLocation loc = Stack->back().second;
----------------
Spurious change?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:8962-8965
+ SourceLocation Loc = D.getDeclSpec().getNoreturnSpecLoc();
+ AttributeCommonInfo Info(SourceRange{Loc},
AttributeCommonInfo::AT_NoReturn,
+ AttributeCommonInfo::AS_Keyword);
+ NewFD->addAttr(::new (Context) C11NoReturnAttr(Context, Info));
----------------
It would be nice if we didn't have to repeat information, if possible. The
previous version of this didn't need to specify `AT_NoReturn` because the
`C11NoReturnAttr` was sufficient to glean that information. It would be awesome
if we could find a way to make this more succinct. Perhaps
`ConcreteAttr::Create(Context, Range, Syntax, Spelling = DefaultValue)` or
something along those lines?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:13880
fmt = "NSString";
- FD->addAttr(FormatAttr::CreateImplicit(Context,
- &Context.Idents.get(fmt),
- FormatIdx+1,
- HasVAListArg ? 0 : FormatIdx+2,
- FD->getLocation()));
+ FD->addAttr(FormatAttr::CreateImplicit(
+ Context, &Context.Idents.get(fmt), FormatIdx + 1,
----------------
Spurious formatting change? Same for the next three changes.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3349
+ AttributeCommonInfo Info{SourceRange(VS.getOverrideLoc()),
+ AttributeCommonInfo::UnknownAttribute,
+ AttributeCommonInfo::AS_Keyword, 0};
----------------
Can we use something other than `UnknownAttribute` here? That implies we don't
know the spelling of the attribute name, which seems wrong.
================
Comment at: clang/lib/Sema/SemaDeclObjC.cpp:4464-4465
// merge the attribute into implementation.
- method->addAttr(
- ObjCRequiresSuperAttr::CreateImplicit(S.Context,
- method->getLocation()));
+ method->addAttr(ObjCRequiresSuperAttr::CreateImplicit(
+ S.Context, method->getLocation()));
}
----------------
Spurious formatting change.
================
Comment at: clang/lib/Sema/SemaObjCProperty.cpp:2414-2415
if (property->hasAttr<NSReturnsNotRetainedAttr>())
- GetterMethod->addAttr(NSReturnsNotRetainedAttr::CreateImplicit(Context,
- Loc));
+ GetterMethod->addAttr(
+ NSReturnsNotRetainedAttr::CreateImplicit(Context, Loc));
----------------
I'll stop pointing out the formatting changes, but you should probably drop
these from the patch.
================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:50
FnScope->setHasFallthroughStmt();
- return ::new (S.Context) auto(Attr);
+ return ::new (S.Context) FallThroughAttr(S.Context, A);
}
----------------
Thank you for this change. I have no idea how the original code made it through
a code review.
================
Comment at: clang/lib/Sema/SemaType.cpp:3942
Attr.setUsedAsTypeAttr();
- return ::new (Ctx)
- AttrT(Attr.getRange(), Ctx, Attr.getAttributeSpellingListIndex());
+ return ::new (Ctx) AttrT(Ctx, Attr);
}
----------------
Since you're touching this code already... can you rename `Attr` to not
conflict with a type name? :-D
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:56
#include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
----------------
Is this still needed?
================
Comment at: clang/lib/Serialization/ASTWriter.cpp:4526
+
+ Record.AddIdentifierRef(A->getAttrName());
+ Record.AddIdentifierRef(A->getScopeName());
----------------
Do we also need bump an AST binary format version number for this sort of
change?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67368/new/
https://reviews.llvm.org/D67368
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
