[PATCH] D92006: Refactoring the attrubute plugin example to fit the new API

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/examples/Attribute/Attribute.cpp:26
   ExampleAttrInfo() {
-    // Can take an optional string argument (the check that the argument
-    // actually is a string happens in handleDeclAttribute).
-    OptArgs = 1;
+    // Can take variadic arguments.
+    OptArgs = 15;
----------------
The comment is a bit misleading, how about: `Can take up to 15 optional arguments, to emulate accepting a variadic number of arguments.`?


================
Comment at: clang/examples/Attribute/Attribute.cpp:57
     }
-    // Check if we have an optional string argument.
-    StringRef Str = "";
+    // Make sure there are at most three arguments exists
+    if (Attr.getNumArgs() > 3) {
----------------
This comment describes what the code is doing but not why. How about: `The example attribute supports up to 15 optional arguments, but this demonstrates how to test for a specific number of arguments.` or something along those lines?


================
Comment at: clang/examples/Attribute/Attribute.cpp:65
+    }
+    // If has arguments, the first argument should be a string literal
+    Expr *Arg0 = nullptr;
----------------
If has arguments -> If there are arguments

(And add a full stop to the end of the comment.)


================
Comment at: clang/examples/Attribute/Attribute.cpp:81
+    D->addAttr(AnnotateAttr::Create(S.Context, "example", &Arg0,
+                                    Attr.getNumArgs(), Attr.getRange()));
     return AttributeApplied;
----------------
This looks dangerous to me -- if there are two or three arguments supplied, then this code will result in a buffer overrun.


================
Comment at: clang/test/Frontend/plugin-attribute.cpp:1
-// RUN: %clang -fplugin=%llvmshlibdir/Attribute%pluginext -emit-llvm -S %s -o - 2>&1 | FileCheck %s --check-prefix=ATTRIBUTE
-// RUN: not %clang -fplugin=%llvmshlibdir/Attribute%pluginext -emit-llvm -DBAD_ATTRIBUTE -S %s -o - 2>&1 | FileCheck %s --check-prefix=BADATTRIBUTE
+// RUN: %clang -fplugin=%llvmshlibdir/Attribute%pluginext -DGOOD_ATTR -fsyntax-only -Xclang -verify=goodattr %s
+// RUN: %clang -fplugin=%llvmshlibdir/Attribute%pluginext -DBAD_ATTR -fsyntax-only -Xclang -verify=badattr %s
----------------
I don't think we need a GOOD_ATTR and BAD_ATTR run line any longer. Instead, you can use a single RUN line with `-verify` (no `=`) and use `expected-error` and `expected-warning` on the lines that need the diagnostics. This means you can also get rid of the `@ line number` constructs and just write the expected comments on the source lines with diagnostics.


================
Comment at: clang/test/Frontend/plugin-attribute.cpp:12
-
-// ATTRIBUTE: [[STR1_VAR:@.+]] = private unnamed_addr constant [10 x i8] c"example()\00"
-// ATTRIBUTE: [[STR2_VAR:@.+]] = private unnamed_addr constant [20 x i8] c"example(somestring)\00"
----------------
A downside to this is that we lose the verification that the plugin actually attaches the attribute to the AST node. Another solution that doesn't involve generating LLVM bitcode from a Frontend test would be to use `-ast-dump` and check the AST nodes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92006