[PATCH] D30009: Add support for '#pragma clang attribute'
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Mar 18 10:22:51 PDT 2017
aaron.ballman added a comment.
I really like the way this feature is shaping up! I apologize for how long it took to review the code (I was out for work for two weeks and this is a pretty extensive patch). Thank you for the continued efforts on this.
================
Comment at: docs/LanguageExtensions.rst:2323
+follow the pragma receive the attributes that are on the attribute stack, until
+the stack is cleared using a ``#pragma clang attribute pop`` directive.
+
----------------
Might want to mention explicitly that these directives nest.
================
Comment at: docs/LanguageExtensions.rst:2337
+The attributes can also be written using the C++11 style syntax, as long
+as just one attribute is specified in the square brackets:
+
----------------
just -> only
================
Comment at: docs/LanguageExtensions.rst:2352
+depends on the subject match rules that were specified in the pragma. Subject
+match rules are specified after the attribute. Firstly, the compiler expects an
+identifier that corresponds to the subject set specifier. The ``apply_to``
----------------
Drop the "Firstly, ".
================
Comment at: docs/LanguageExtensions.rst:2356
+**strict** subject set. Strict subject sets have to be identical to the
+attribute's own subject set, otherwise a compilation error is presented. For
+example, an attribute like ``[[noreturn]]`` whose subject set is just
----------------
"otherwise it results in an error." ?
================
Comment at: docs/LanguageExtensions.rst:2378
+ // This is an error as well, [[noreturn]] must be applied *only* to function:
+ #pragma clang attribute push([[noreturn]], apply_to = any(function, variable))
+
----------------
We don't describe `any` below, but should.
================
Comment at: docs/LanguageExtensions.rst:2387
+The ``apply_to`` specifier should be used whenever one wants to apply an
+attribute to all of the declaration that can receive that attribute. The use
+of ``apply_to`` ensures that future versions of compiler will report an error
----------------
declarations (plural).
================
Comment at: docs/LanguageExtensions.rst:2388
+attribute to all of the declaration that can receive that attribute. The use
+of ``apply_to`` ensures that future versions of compiler will report an error
+if the attribute will start supporting a new kind of declaration.
----------------
of compiler -> of the compiler
================
Comment at: docs/LanguageExtensions.rst:2389
+of ``apply_to`` ensures that future versions of compiler will report an error
+if the attribute will start supporting a new kind of declaration.
+
----------------
will start supporting -> supports
================
Comment at: docs/LanguageExtensions.rst:2430
+
+- ``function(is_method)``: Can be used to apply attributes to C++ methods,
+ including operators and constructors/destructors.
----------------
Instead of `is_method`, I think we should use `is_instance`, `is_member`, or something else. C++ doesn't really use the terminology "method", but Objective-C does, which is why this could be confusing. It might also be good to mention the behavior of a static member functions.
Whatever terminology gets picked, you should use that phrasing wherever you say "C++ method".
================
Comment at: docs/LanguageExtensions.rst:2435
+ methods, and variables/fields whose type is a function pointer. It does not
+ apply attributes to Objective-C methods or blocks!
+
----------------
Replace the exclamation point with a full stop.
================
Comment at: docs/LanguageExtensions.rst:2440
+
+- ``record``: Can be used to apply attributes to ``struct``, ``class`` and
+ ``union`` declarations.
----------------
Insert a comma after class.
================
Comment at: docs/LanguageExtensions.rst:2443
+
+- ``record(unless(is_union))``: Can be used to apply attributes only to
+ ``struct`` and ``class`` declarations.
----------------
Does unless() work in other combinations? e.g., can I use record(unless(is_enum))? I'm not saying that it has to, but it would be good to document one way or the other.
================
Comment at: docs/LanguageExtensions.rst:2448
+
+- ``enum_case``: Can be used to apply attributes to enumerators.
+
----------------
I'd prefer these to be named `enumeration` and `enumerator`, `enum` and `enum_constant`, or something. `enum_case` is too novel of a term for me.
================
Comment at: docs/LanguageExtensions.rst:2451
+- ``variable``: Can be used to apply attributes to variables, including
+ local variables, parameters and global variables.
+
----------------
Comma after parameters.
================
Comment at: docs/LanguageExtensions.rst:2462
+
+- ``variable(unless(is_parameter))``: Can be used to apply attributes to all
+ the variables that are not parameters.
----------------
Same question here as above with the unless(), can I specify any is_foo there?
================
Comment at: docs/LanguageExtensions.rst:2486
+
+- ``block``: Can be used to apply attributes to block declarations. This does
+ not include variables/fields of block pointer type.
----------------
Perhaps name this one `objc_block` to be consistent with the other objective-c features?
================
Comment at: docs/LanguageExtensions.rst:2497
+:doc:`individual documentation for that attribute <AttributeReference>`.
+
+The attributes are applied to a declaration only when there would
----------------
We should also support __declspec attributes shouldn't we (I see no reason to support 2 out of the 3 attribute styles)? If so, the docs should state that, and if not, I'd like to understand why and the docs should still state that.
================
Comment at: docs/LanguageExtensions.rst:2502
+unless the first declaration failed to receive the attribute because of a
+ompilation error. The attributes that aren't applied to any declaration are not
+verified semantically.
----------------
Typo: compilation error
================
Comment at: include/clang/Basic/Attr.td:288
+// the NonParmVar attribute subject.
+class AttrSubjectMatcherSubRule<string name,list<AttrSubject> subjects,
+ bit negated = 0> {
----------------
space after the comma before list.
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:999-1002
+def err_pragma_attribute_expected_subject_sub_identifier_supported : Error<
+ "expected an identifier that corresponds to an attribute subject matcher "
+ "sub-rule; '%0' matcher supports the following sub-rules: %1">;
+def err_pragma_attribute_expected_subject_sub_identifier_unsupported : Error<
----------------
I would combine these two and use a %select to distinguish between them.
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1005-1008
+def err_pragma_attribute_unknown_subject_sub_rule_supported : Error<
+ "unknown attribute subject matcher sub-rule '%0'; '%1' matcher supports "
+ "the following sub-rules: %2">;
+def err_pragma_attribute_unknown_subject_sub_rule_unsupported : Error<
----------------
Same for these.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:752
+def err_pragma_attribute_invalid_strict_subject_specifier : Error<
+ "use of 'apply_to' subject set specifier for an attribute without a strict "
+ "subject set">;
----------------
subject set -> attribute subject set (since you used that phrasing in another diagnostic and the documentation).
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:761-763
+def err_pragma_attribute_invalid_matchers : Error<
+ "attribute %0 can't be applied to %1">;
+def err_pragma_attribute_missing_matchers : Error<
----------------
I'd combine these as well.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:766
+def err_pragma_attribute_stack_mismatch : Error<
+ "'#pragma clang attribute pop' with no matching '#pragma clang attribute push'">;
+
----------------
We should have a test for a file that has an unmatched push as well. e.g.,
```
#pragma clang attribute push(annotate("blah"), apply_only_to=function)
\EOF
```
================
Comment at: include/clang/Sema/Sema.h:8146
+ /// \brief Called on well formed '\#pragma clang attribute push'.
+ void ActOnPragmaAttributePush(AttributeList &Attribute,
----------------
well formed -> well-formed
================
Comment at: include/clang/Sema/Sema.h:8155
+
+ /// \brief Called on well formed '\#pragma clang attribute pop'.
+ void ActOnPragmaAttributePop(SourceLocation PragmaLoc);
----------------
well formed -> well-formed
================
Comment at: include/clang/Sema/Sema.h:8160
+ /// '\#pragma clang attribute push' directives to the given declaration.
+ void AddPragmaAttributes(Scope *S, Decl *D);
+
----------------
I worry about new calls to ProcessDeclAttributes() that don't have a nearby call to AddPragmaAttributes(). I wonder if there's a way that we can combat that likely misuse?
================
Comment at: lib/Parse/ParsePragma.cpp:1012
+ }
+ auto Rule = isAttributeSubjectMatchRule(Name);
+ if (!Rule.first) {
----------------
Please don't use `auto` here, since the type is not immediately obvious from the initialization and it's not something trivially easy to figure out, like an iterator.
================
Comment at: lib/Parse/ParsePragma.cpp:1060-1076
+ SubRuleName = getIdentifier(Tok);
+ if (SubRuleName.empty()) {
+ if (const char *SubRules =
+ validAttributeSubjectMatchSubRules(PrimaryRule))
+ Diag(
+ SubRuleLoc,
+ diag::
----------------
Can this duplicate code be factored into a helper function? That might also help with the crazy formatting of the calls to `Diag()`, too. Same suggestion applies below.
================
Comment at: lib/Parse/ParsePragma.cpp:1135
+void Parser::HandlePragmaAttribute() {
+ assert(Tok.is(tok::annot_pragma_attribute));
+ SourceLocation PragmaLoc = Tok.getLocation();
----------------
It's usually good practice to have the && "This is what went wrong" text for all the asserts. Same comment applies elsewhere.
================
Comment at: lib/Parse/ParsePragma.cpp:1158
+ Diag(Loc, diag::err_pragma_attribute_multiple_attributes);
+ Attrs.getList()->setNext(nullptr);
+ }
----------------
Is this going to cause a memory leak for those attributes that were parsed but dropped?
================
Comment at: lib/Parse/ParsePragma.cpp:1201
+ }
+ bool IsStrict = II->isStr("apply_to");
+ SourceLocation SubjectSetSpecifierLoc = ConsumeToken();
----------------
Might as well cache this above.
================
Comment at: lib/Parse/ParsePragma.cpp:2556
+/// \code
+/// #pragma clang attribute push(attribute, subject-set)
+/// #pragma clang attribute pop
----------------
You should mention where the reader can find more information about subject-set.
================
Comment at: lib/Parse/ParsePragma.cpp:2590
+ if (Info->Action == PragmaAttributeInfo::Push) {
+ if (Tok.isNot(tok::l_paren)) {
+ PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;
----------------
Not that this impacts your patch, but we really should modify the BalancedDelimiterTracker to work for pragmas so that we don't have to do this dance manually.
================
Comment at: lib/Sema/SemaAttr.cpp:401
+
+static CharSourceRange replacementRangeForListElement(const Sema &S,
+ SourceRange Range) {
----------------
Why not move these static functions into the anonymous namespace (and remove the static)?
================
Comment at: lib/Sema/SemaAttr.cpp:419
+ if (I.Index)
+ OS << (I.Index == Rules.size() - 1 ? " and " : ", ");
+ OS << "'" << attr::getSubjectMatchRuleSpelling(I.Value) << "'";
----------------
I think it should be `", and "`.
The down-side to having "and" in here is that it makes it harder to localize the diagnostics, but I think it's fine in this instance. I only mention it in case you have a better idea.
================
Comment at: lib/Sema/SemaAttr.cpp:477
+ for (const auto &Rule : Rules) {
+ auto ParentRule = getParentAttrMatcherRule(Rule.first);
+ if (!ParentRule)
----------------
Don't use `auto`.
================
Comment at: lib/Sema/SemaAttr.cpp:499
+ for (const auto &Rule : Rules) {
+ auto ParentRule = getParentAttrMatcherRule(Rule.first);
+ if (!ParentRule)
----------------
Don't use `auto`.
================
Comment at: lib/Sema/SemaAttr.cpp:557
+ Diagnostic << FixItHint::CreateInsertion(FirstRuleLoc, "any(");
+ auto MissingCode = attrMatcherRuleListToCodeString(MissingRules);
+ if (AnyLoc.isInvalid())
----------------
Don't use `auto`.
================
Comment at: lib/Sema/SemaAttr.cpp:565-566
+ // Keep going but pretend the missing rules where specified.
+ for (const auto &Rule : MissingRules)
+ SubjectMatchRules.push_back(Rule);
+ }
----------------
Can use use std::copy() with a back_inserter.
================
Comment at: lib/Sema/SemaAttr.cpp:602-603
+ PragmaAttributeStack.pop_back();
+ // FIXME: Should we check the attribute even when it's not applied to any
+ // declaration?
+}
----------------
I think that makes sense (I can imagine someone writing the pragma push and pop before writing all their declarations), but I might be misunderstanding the question.
================
Comment at: lib/Sema/SemaAttr.cpp:631
+ ProcessDeclAttributeList(S, D, Entry.Attribute);
+ HasFailed = Trap.hasErrorOccurred();
+ }
----------------
Do we only care about errors, or should we also care about warnings? Incorrect attributes that do not affect code generation often are treated as warnings (and the attribute is simply ignored) rather than errors, so the user may want to know about those issues.
================
Comment at: test/Sema/pragma-attribute.c:2
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -fsyntax-only -ast-dump -ast-dump-filter test %s | FileCheck %s
+
----------------
-ast-dump tests live in Misc rather than Sema.
================
Comment at: test/SemaCXX/pragma-attribute-subject-match-rules.cpp:170
+#pragma clang attribute pop
\ No newline at end of file
----------------
Please add the newline (and this test should be in Misc as well).
================
Comment at: test/SemaCXX/pragma-attribute.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -ast-dump -ast-dump-filter test -std=c++11 -DNOERROR %s | FileCheck %s
----------------
This should also be in Misc
================
Comment at: test/SemaObjC/pragma-attribute-subject-match-rules.m:107
+#pragma clang attribute pop
\ No newline at end of file
----------------
Please add the newline, and move the test into Misc.
================
Comment at: test/SemaObjC/pragma-attribute.m:2
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-objc-root-class -ast-dump -ast-dump-filter test %s | FileCheck %s
+
----------------
This test should also be in Misc.
================
Comment at: test/lit.cfg:287
+config.substitutions.append( ('%src_include_dir', config.clang_src_dir + '/include') )
+
----------------
Why is this change (and the other lit change) required?
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1558
+ // overrides the rules options.
+ auto Opts = Constraint->getValueAsListOfDefs("LangOpts");
+ if (!Opts.empty())
----------------
Don't use `auto`.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1639
+ SubjectContainer->getValueAsListOfDefs("Subjects");
+ for (const Record *Subject : ApplicableSubjects) {
+ bool Inserted =
----------------
Can use `const auto *`
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1648
+ };
+ for (const Record *MetaSubject : MetaSubjects) {
+ MapFromSubjectsToRules(MetaSubject, MetaSubject);
----------------
Can use `const auto *`.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1652
+ MetaSubject->getValueAsListOfDefs("Constraints");
+ for (const Record *Constraint : Constraints)
+ MapFromSubjectsToRules(Constraint, MetaSubject, Constraint);
----------------
Can use `const auto *`.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1669
+ OS << "#endif\n";
+ for (const AttributeSubjectMatchRule &Rule : Rules) {
+ OS << (Rule.isSubRule() ? "ATTR_MATCH_SUB_RULE" : "ATTR_MATCH_RULE") << '(';
----------------
Can use `const auto &`.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1711
+ return false;
+ for (const Record *Subject : Subjects) {
+ auto It = SubjectsToRules.find(Subject);
----------------
Can use `const auto *`.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1712-1713
+ for (const Record *Subject : Subjects) {
+ auto It = SubjectsToRules.find(Subject);
+ if (It == SubjectsToRules.end())
+ return false;
----------------
No need to store `It`.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:1738
+ std::vector<Record *> Subjects = SubjectObj->getValueAsListOfDefs("Subjects");
+ for (const Record *Subject : Subjects) {
+ auto It = SubjectsToRules.find(Subject);
----------------
Can use `const auto *`.
Repository:
rL LLVM
https://reviews.llvm.org/D30009
More information about the cfe-commits
mailing list