[PATCH] D95695: [clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added inline comments.


================
Comment at: clang/test/AST/ast-print-attr.c:31
+// CHECK: int fun_annotate() __attribute__((annotate("annotation")))
+int fun_annotate() __attribute__((annotate("annotation")));
----------------
aaron.ballman wrote:
> Can you add a second test that shows we properly print the comma? e.g., `int fun_annotate2() __attribute__((annotate("annotation one", "annotation two")));`
I switched this to use `ownership_holds` and `ownership_returns` because `annotate` has a `VariadicExprArgument` and it doesn't know how to print expressions (it just prints a pointer value). `ownership_holds` and `ownership_returns` are the same attribute, and they have the same bug-triggering configuration (a fixed argument followed by a possibly-empty list):

```c++
int *fun_returns() __attribute__((ownership_returns(fun_returns)));
void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
```

In the `ownership_returns` case, without my change, Clang prints `__attribute__((ownership_returns(foo, )));`.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2252
+  // Helper to print the starting character of an attribute argument. If there
+  // hasn't been an argument yet, it prints an opening parenthese; otherwise it
+  // prints a comma.
----------------
aaron.ballman wrote:
> One downside to printing the opening paren is that this can't be used in a generic way for generating any comma-separate list. That said, I think this functionality is clean -- perhaps renaming the function from `Comma` to `PrintAttributeArgListElement` or something would be an improvement?
I went with `DelimitAttributeArgument`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95695/new/

https://reviews.llvm.org/D95695