aaron.ballman added a subscriber: cfe-commits.
aaron.ballman added reviewers: rsmith, majnemer.
aaron.ballman added a comment.
I don't have the expertise to review the semantics of the attribute, but here
are some cursory thoughts on the attribute itself.
================
Comment at: include/clang/Basic/Attr.td:355
@@ +354,3 @@
+ let Args = [VariadicStringArgument<"Tags">];
+ let Subjects = SubjectList<[Struct, Var, Function, Namespace], ErrorDiag,
+
"ExpectedStructClassVariableFunctionMethodOrInlineNamespace">;
----------------
The diagnostic doesn't match the subject -- "methods" refers to Objective-C
methods, which should either be listed in the SubjectList or removed from the
diagnostic.
================
Comment at: include/clang/Basic/Attr.td:357
@@ +356,3 @@
+
"ExpectedStructClassVariableFunctionMethodOrInlineNamespace">;
+ let Documentation = [Undocumented];
+}
----------------
No new undocumented attributes, please -- you should add documentation to
AttrDocs.td and link to it from here.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4149
@@ +4148,3 @@
+def err_attr_abi_tag_only_on_inline_namespace :
+ Error<"abi_tag attribute only allowed on inline namespaces">;
+def err_attr_abi_tag_only_on_named_namespace :
----------------
Should quote 'abi_tag' in the diagnostic (here and below as well). Also, why an
error instead of a warning and dropping the attribute from the declaration?
Perhaps: 'abi_tag' attribute on %select{inline|anonymous}0 namespace ignored"
as a warning, and remove the below diagnostic entirely?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4456
@@ +4455,3 @@
+
+ SmallVector<std::string, 4> Tags;
+
----------------
Why a vector of std::string instead of StringRef?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4476
@@ +4475,3 @@
+ if (NS && Attr.getNumArgs() == 0) {
+ Tags.push_back(NS->getName());
+ }
----------------
Indentation is incorrect; also, elide braces for single statement ifs.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4479
@@ +4478,3 @@
+
+ // store tags sorted and without duplicates
+ std::sort(Tags.begin(), Tags.end());
----------------
Comments should be complete sentences (with capitalization and punctuation);
here and elsewhere.
Repository:
rL LLVM
http://reviews.llvm.org/D12834
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
