[Attr] Fix paren, comma, and omitted arg printing in some cases

[jdenny]

Summary

The first FIXME introduced here will be addressed in another patch
(https://reviews.llvm.org/D43248).

[aaron.ballman]

In general, I think the idea is reasonable, but in practice I'm worried about the attributes with custom parsing. Sometimes the parsing requires those extra bits and I'm not certain of how best to address that.

test/Misc/ast-print-objectivec.m

52 ↗

(On Diff #135836)

This fix is incorrect in this case -- the attribute doesn't parse without the extra commas.

https://godbolt.org/g/Ze69HD

[jdenny]

test/Misc/ast-print-objectivec.m

52 ↗

(On Diff #135836)

Sorry, I should have researched that one.

It is straight-forward to make it treat an argument as provided based on its argument type. Will look into it soon.

[jdenny]

Aaron: Because the last two arguments for objc_bridge_related must be delimited with commas, this revision takes the view that they are not optional but are permitted to be the empty string.

The test suite behaves as expected. Are there any untested attributes that might be broken by this patch? Hopefully they can be addressed in a similar manner.

[aaron.ballman]

test/Sema/attr-print.c

12

Please add a test showing that objc_bridge_related isn't mangled by -ast-print.

utils/TableGen/ClangAttrEmitter.cpp

303–304

I think you can remove this case and the TypeSourceInfo * as well and just rely on returning "false" in those cases.

[jdenny]

test/Sema/attr-print.c

12

There's already one in Misc/ast-print-objectivec.m, but I can add one here too if that's helpful.

[aaron.ballman]

test/Sema/attr-print.c

12

Nope, not necessary -- it's already got the coverage I was hoping for. Thank you!

[jdenny]

This addresses the comment about redundant ifs.

[aaron.ballman]

LGTM! I can commit on your behalf, or, if you plan to continue submitting patches, you can ask for commit privileges (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) and commit yourself. Either way works for me.

[jdenny]

In D43748#1021246, @aaron.ballman wrote:

LGTM! I can commit on your behalf, or, if you plan to continue submitting patches, you can ask for commit privileges (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) and commit yourself. Either way works for me.

I'll definitely pursue commit privileges. If you don't mind committing anything you accept today, that would be great. Thanks for your help!

[aaron.ballman]

Committed in r326266.

include/clang/Basic/Attr.td

def ObjCBridgeMutable : InheritableAttr {
   let Args = [IdentifierArgument<"BridgedType">];
   let Documentation = [Undocumented];
 }
 
 def ObjCBridgeRelated : InheritableAttr {
   let Spellings = [Clang<"objc_bridge_related">];
   let Subjects = SubjectList<[Record], ErrorDiag>;
   let Args = [IdentifierArgument<"RelatedClass">,
-          IdentifierArgument<"ClassMethod", 1>,
-          IdentifierArgument<"InstanceMethod", 1>];
+          IdentifierArgument<"ClassMethod">,
+          IdentifierArgument<"InstanceMethod">];
   let HasCustomParsing = 1;
   let Documentation = [Undocumented];
 }
 
 def NSReturnsRetained : InheritableAttr {
   let Spellings = [Clang<"ns_returns_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
   let Documentation = [Undocumented];

lib/Parse/ParseDecl.cpp

if (Tok.isNot(tok::identifier)) {
     return;
   }
   IdentifierLoc *RelatedClass = ParseIdentifierLoc();
   if (ExpectAndConsume(tok::comma)) {
     SkipUntil(tok::r_paren, StopAtSemi);
     return;
   }
 
-  // Parse optional class method name.
+  // Parse class method name.  It's non-optional in the sense that a trailing
+  // comma is required, but it can be the empty string, and then we record a
+  // nullptr.
   IdentifierLoc *ClassMethod = nullptr;
   if (Tok.is(tok::identifier)) {
     ClassMethod = ParseIdentifierLoc();
     if (!TryConsumeToken(tok::colon)) {
       Diag(Tok, diag::err_objcbridge_related_selector_name);
       SkipUntil(tok::r_paren, StopAtSemi);
       return;
     }
   }
   if (!TryConsumeToken(tok::comma)) {
     if (Tok.is(tok::colon))
       Diag(Tok, diag::err_objcbridge_related_selector_name);
     else
       Diag(Tok, diag::err_expected) << tok::comma;
     SkipUntil(tok::r_paren, StopAtSemi);
     return;
   }
   
-  // Parse optional instance method name.
+  // Parse instance method name.  Also non-optional but empty string is
+  // permitted.
   IdentifierLoc *InstanceMethod = nullptr;
   if (Tok.is(tok::identifier))
     InstanceMethod = ParseIdentifierLoc();
   else if (Tok.isNot(tok::r_paren)) {
     Diag(Tok, diag::err_expected) << tok::r_paren;
     SkipUntil(tok::r_paren, StopAtSemi);
     return;
   }

test/Sema/attr-print.c

 // RUN: %clang_cc1 %s -ast-print -fms-extensions | FileCheck %s
 
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
 // FIXME: Print this at a valid location for a __declspec attr.
 // CHECK: int y __declspec(align(4));
 __declspec(align(4)) int y;
 
+// CHECK: short arr[3] __attribute__((aligned));
+short arr[3] __attribute__((aligned));
+
 // CHECK: void foo() __attribute__((const));
 void foo() __attribute__((const));
 
 // CHECK: void bar() __attribute__((__const));
 void bar() __attribute__((__const));
 
 // CHECK: int * __ptr32 p32;
 int * __ptr32 p32;

test/Sema/attr-print.cpp

+// RUN: %clang_cc1 %s -ast-print | FileCheck %s
+
+// CHECK: void *as2(int, int) __attribute__((alloc_size(1, 2)));
+void *as2(int, int) __attribute__((alloc_size(1, 2)));
+// CHECK: void *as1(void *, int) __attribute__((alloc_size(2)));
+void *as1(void *, int) __attribute__((alloc_size(2)));

utils/TableGen/ClangAttrEmitter.cpp

public:
     virtual void writeCtorBody(raw_ostream &OS) const {}
     virtual void writeCtorInitializers(raw_ostream &OS) const = 0;
     virtual void writeCtorDefaultInitializers(raw_ostream &OS) const = 0;
     virtual void writeCtorParameters(raw_ostream &OS) const = 0;
     virtual void writeDeclarations(raw_ostream &OS) const = 0;
     virtual void writePCHReadArgs(raw_ostream &OS) const = 0;
     virtual void writePCHReadDecls(raw_ostream &OS) const = 0;
     virtual void writePCHWrite(raw_ostream &OS) const = 0;
+    virtual std::string getIsOmitted() const { return "false"; }
     virtual void writeValue(raw_ostream &OS) const = 0;
     virtual void writeDump(raw_ostream &OS) const = 0;
     virtual void writeDumpChildren(raw_ostream &OS) const {}
     virtual void writeHasChildren(raw_ostream &OS) const { OS << "false"; }
 
     virtual bool isEnumArg() const { return false; }
     virtual bool isVariadicEnumArg() const { return false; }
     virtual bool isVariadic() const { return false; }
void writePCHReadArgs(raw_ostream &OS) const override {
       OS << getLowerName();
     }
 
     void writePCHWrite(raw_ostream &OS) const override {
       OS << "    " << WritePCHRecord(type, "SA->get" +
                                            std::string(getUpperName()) + "()");
     }
 
+    std::string getIsOmitted() const override {
+      if (type == "IdentifierInfo *")
+        return "!get" + getUpperName().str() + "()";
+      // FIXME: Do this declaratively in Attr.td.
+      if (getAttrName() == "AllocSize")
+        return "0 == get" + getUpperName().str() + "()";
+      return "false";
+    }
+
     void writeValue(raw_ostream &OS) const override {
-      if (type == "FunctionDecl *") {
+      if (type == "FunctionDecl *")
         OS << "\" << get" << getUpperName()
            << "()->getNameInfo().getAsString() << \"";
-      } else if (type == "IdentifierInfo *") {
-        OS << "\";\n";
-        if (isOptional())
-          OS << "    if (get" << getUpperName() << "()) ";
-        else
-          OS << "    ";
-        OS << "OS << get" << getUpperName() << "()->getName();\n";
-        OS << "    OS << \"";
-      } else if (type == "TypeSourceInfo *") {
+      else if (type == "IdentifierInfo *")
+        // Some non-optional (comma required) identifier arguments can be the
+        // empty string but are then recorded as a nullptr.
+        OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
+           << "()->getName() : \"\") << \"";
+      else if (type == "TypeSourceInfo *")
         OS << "\" << get" << getUpperName() << "().getAsString() << \"";
-      } else {
+      else
         OS << "\" << get" << getUpperName() << "() << \"";
-      }
-    }
 
     void writeDump(raw_ostream &OS) const override {
       if (type == "FunctionDecl *" || type == "NamedDecl *") {
         OS << "    OS << \" \";\n";
         OS << "    dumpBareDeclRef(SA->get" << getUpperName() << "());\n"; 
       } else if (type == "IdentifierInfo *") {
-        if (isOptional())
-          OS << "    if (SA->get" << getUpperName() << "())\n  ";
-        OS << "    OS << \" \" << SA->get" << getUpperName()
+        // Some non-optional (comma required) identifier arguments can be the
+        // empty string but are then recorded as a nullptr.
+        OS << "    if (SA->get" << getUpperName() << "())\n"
+           << "      OS << \" \" << SA->get" << getUpperName()
            << "()->getName();\n";
       } else if (type == "TypeSourceInfo *") {
         OS << "    OS << \" \" << SA->get" << getUpperName()
            << "().getAsString();\n";
       } else if (type == "bool") {
         OS << "    if (SA->get" << getUpperName() << "()) OS << \" "
            << getUpperName() << "\";\n";
       } else if (type == "int" || type == "unsigned") {
void writePCHWrite(raw_ostream &OS) const override {
       OS << "    Record.push_back(SA->is" << getUpperName() << "Expr());\n";
       OS << "    if (SA->is" << getUpperName() << "Expr())\n";
       OS << "      Record.AddStmt(SA->get" << getUpperName() << "Expr());\n";
       OS << "    else\n";
       OS << "      Record.AddTypeSourceInfo(SA->get" << getUpperName()
          << "Type());\n";
     }
 
+    std::string getIsOmitted() const override {
+      return "!is" + getLowerName().str() + "Expr || !" + getLowerName().str()
+             + "Expr";
+    }
+
     void writeValue(raw_ostream &OS) const override {
       OS << "\";\n";
-      // The aligned attribute argument expression is optional.
-      OS << "    if (is" << getLowerName() << "Expr && "
-         << getLowerName() << "Expr)\n";
-      OS << "      " << getLowerName() << "Expr->printPretty(OS, nullptr, Policy);\n";
+      OS << "    " << getLowerName()
+         << "Expr->printPretty(OS, nullptr, Policy);\n";
       OS << "    OS << \"";
     }
 
     void writeDump(raw_ostream &OS) const override {}
 
     void writeDumpChildren(raw_ostream &OS) const override {
       OS << "    if (SA->is" << getUpperName() << "Expr())\n";
       OS << "      dumpStmt(SA->get" << getUpperName() << "Expr());\n";
if (Variety == "Pragma") {
       OS << "\";\n";
       OS << "    printPrettyPragma(OS, Policy);\n";
       OS << "    OS << \"\\n\";";
       OS << "    break;\n";
       OS << "  }\n";
       continue;
     }
 
-    // Fake arguments aren't part of the parsed form and should not be
-    // pretty-printed.
-    bool hasNonFakeArgs = llvm::any_of(
-        Args, [](const std::unique_ptr<Argument> &A) { return !A->isFake(); });
-
-    // FIXME: always printing the parenthesis isn't the correct behavior for
-    // attributes which have optional arguments that were not provided. For
-    // instance: __attribute__((aligned)) will be pretty printed as
-    // __attribute__((aligned())). The logic should check whether there is only
-    // a single argument, and if it is optional, whether it has been provided.
-    if (hasNonFakeArgs)
-      OS << "(";
     if (Spelling == "availability") {
+      OS << "(";
       writeAvailabilityValue(OS);
+      OS << ")";
     } else if (Spelling == "deprecated" || Spelling == "gnu::deprecated") {
-        writeDeprecatedAttrValue(OS, Variety);
+      OS << "(";
+      OS << ")";
     } else {
-      unsigned index = 0;
+      // To avoid printing parentheses around an empty argument list or
+      // printing spurious commas at the end of an argument list, we need to
+      // determine where the last provided non-fake argument is.
+      unsigned NonFakeArgs = 0;
+      unsigned TrailingOptArgs = 0;
+      bool FoundNonOptArg = false;
+      for (const auto &arg : llvm::reverse(Args)) {
+        if (arg->isFake())
+          continue;
+        ++NonFakeArgs;
+        if (FoundNonOptArg)
+          continue;
+        // FIXME: arg->getIsOmitted() == "false" means we haven't implemented
+        // any way to detect whether the argument was omitted.
+        if (!arg->isOptional() || arg->getIsOmitted() == "false") {
+          FoundNonOptArg = true;
+          continue;
+        }
+        if (!TrailingOptArgs++)
+          OS << "\";\n"
+             << "    unsigned TrailingOmittedArgs = 0;\n";
+        OS << "    if (" << arg->getIsOmitted() << ")\n"
+           << "      ++TrailingOmittedArgs;\n";
+      }
+      if (TrailingOptArgs)
+        OS << "    OS << \"";
+      if (TrailingOptArgs < NonFakeArgs)
+        OS << "(";
+      else if (TrailingOptArgs)
+        OS << "\";\n"
+           << "    if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
+           << "       OS << \"(\";\n"
+           << "    OS << \"";
+      unsigned ArgIndex = 0;
       for (const auto &arg : Args) {
-        if (arg->isFake()) continue;
-        if (index++) OS << ", ";
+        if (arg->isFake())
+          continue;
+        if (ArgIndex) {
+          if (ArgIndex >= NonFakeArgs - TrailingOptArgs)
+            OS << "\";\n"
+               << "    if (" << ArgIndex << " < " << NonFakeArgs
+               << " - TrailingOmittedArgs)\n"
+               << "      OS << \", \";\n"
+               << "    OS << \"";
+          else
+            OS << ", ";
+        }
+        std::string IsOmitted = arg->getIsOmitted();
+        if (arg->isOptional() && IsOmitted != "false")
+          OS << "\";\n"
+             << "    if (!(" << IsOmitted << ")) {\n"
+             << "      OS << \"";
         arg->writeValue(OS);
+        if (arg->isOptional() && IsOmitted != "false")
+          OS << "\";\n"
+             << "    }\n"
+             << "    OS << \"";
+        ++ArgIndex;
       }
+      if (TrailingOptArgs < NonFakeArgs)
+        OS << ")";
+      else if (TrailingOptArgs)
+        OS << "\";\n"
+           << "    if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
+           << "       OS << \")\";\n"
+           << "    OS << \"";
     }
 
-    if (hasNonFakeArgs)
-      OS << ")";
     OS << Suffix + "\";\n";
 
     OS <<
       "    break;\n"
       "  }\n";
   }
 
   // End of the switch statement.