[PATCH] D12834: add gcc abi_tag support

[Aaron Ballman via cfe-commits]

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