[PATCH] D18035: [GCC] PR23529 Mangler part of attrbute abi_tag support

[David Majnemer via cfe-commits]

majnemer added inline comments.

================
Comment at: lib/AST/ItaniumMangle.cpp:217-218
@@ -216,2 +216,4 @@
   raw_ostream &Out;
+  bool NullOut = false;
+  bool DisableDerivedAbiTags = false;
 
----------------
Please add a comment describe this state.

================
Comment at: lib/AST/ItaniumMangle.cpp:329
@@ +328,3 @@
+
+    bool LinkActive{ false };
+
----------------
Please use = instead of list initialization.

================
Comment at: lib/AST/ItaniumMangle.cpp:332
@@ +331,3 @@
+  public:
+    explicit AbiTagState(AbiTagState *&linkHead) : LinkHead(linkHead) {
+      Parent = LinkHead;
----------------
Variables start with an uppercase letter.

================
Comment at: lib/AST/ItaniumMangle.cpp:339-340
@@ +338,4 @@
+    // no copy, no move
+    AbiTagState(AbiTagState const &) = delete;
+    AbiTagState &operator=(AbiTagState const &) = delete;
+
----------------
const typically goes before the type.

================
Comment at: lib/AST/ItaniumMangle.cpp:363
@@ +362,3 @@
+
+      if (dyn_cast<FunctionDecl>(ND) || dyn_cast<VarDecl>(ND)) {
+        // assert(AdditionalAbiTags && "function and variables need a list of
----------------
Don't use dyn_cast if you aren't going to use the value, use isa instead.

================
Comment at: lib/AST/ItaniumMangle.cpp:364-365
@@ +363,4 @@
+      if (dyn_cast<FunctionDecl>(ND) || dyn_cast<VarDecl>(ND)) {
+        // assert(AdditionalAbiTags && "function and variables need a list of
+        // additional abi tags");
+      } else {
----------------
commented out code?

================
Comment at: lib/AST/ItaniumMangle.cpp:394
@@ +393,3 @@
+          if (std::find(TagList.begin(), TagList.end(), Tag) == TagList.end()) {
+            // don't insert duplicates
+            TagList.push_back(Tag);
----------------
Sentences in LLVM comments start with a capital letter and end with a period.

================
Comment at: lib/AST/ItaniumMangle.cpp:427
@@ +426,3 @@
+  AbiTagState *AbiTags = nullptr;
+  AbiTagState AbiTagsRoot{ AbiTags };
+
----------------
Is this clang-format'd?

================
Comment at: lib/AST/ItaniumMangle.cpp:670-671
@@ +669,4 @@
+                                  const AbiTagList *AdditionalAbiTags) {
+  assert(AbiTags && "require AbiTagState");
+  if (AbiTags)
+    AbiTags->write(Out, ND,
----------------
Why do you have an assert and check for that the pointer is not null?

================
Comment at: lib/AST/ItaniumMangle.cpp:824
@@ +823,3 @@
+  if (!ExcludeUnqualifiedName) {
+    if (const VarDecl *VD = dyn_cast<VarDecl>(ND)) {
+      AbiTagList VariableAdditionalAbiTags = makeAdditionalTagsForVariable(VD);
----------------
Use `const auto *` if the type is obvious from the RHS.

================
Comment at: lib/AST/ItaniumMangle.cpp:1447
@@ +1446,3 @@
+  {
+    AbiTagState localAbiTags(AbiTags);
+
----------------
Variables start with an upper case letter.

================
Comment at: lib/AST/ItaniumMangle.cpp:4641-4642
@@ -4218,2 +4640,4 @@
   CXXNameMangler Mangler(*this, Out);
+  // GCC 5.3.0 doesn't emit derived abi tags for but that seems to be a bug
+  // that is fixed in trunk.
   Mangler.getStream() << "_ZGV";
----------------
I think a noun is missing between "for" and "but".


http://reviews.llvm.org/D18035