[PATCH] D99861: [Clang] Record tokens in attribute arguments for user-defined C++/C2x attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rsmith, erichkeane.
aaron.ballman added a comment.

Thank you for this patch, I think it's really useful functionality for plugin authors!

Adding some additional reviewers for more opinions on the changes in the preprocessor.



================
Comment at: clang/examples/PrintAttributeTokens/CMakeLists.txt:3-10
+if( NOT MSVC ) # MSVC mangles symbols differently, and
+               # PrintAttributeTokens.export contains C++ symbols.
+  if( NOT LLVM_REQUIRES_RTTI )
+    if( NOT LLVM_REQUIRES_EH )
+      set(LLVM_EXPORTED_SYMBOL_FILE ${CMAKE_CURRENT_SOURCE_DIR}/PrintAttributeTokens.exports)
+    endif()
+  endif()
----------------
I'm not certain what this is actually doing -- the .exports file is empty (and no other plugin has this sort of .exports file thing). Is this needed?


================
Comment at: clang/examples/PrintAttributeTokens/PrintAttributeTokens.cpp:37
+                                   const ParsedAttr &Attr) const override {
+    llvm::errs() << "PrintAttributeTokens -----------------------\n";
+    D->dump(llvm::errs());
----------------
Should probably use `llvm::outs()` instead (here and elsewhere in the plugin).


================
Comment at: clang/examples/PrintAttributeTokens/PrintAttributeTokens.cpp:44-45
+    } else {
+      const Token *tokens = Attr.getTokens();
+      for (unsigned i = 0; i < Attr.getNumTokens(); i++) {
+        llvm::errs() << tokens[i].getName();
----------------
Per the usual coding conventions.


================
Comment at: clang/examples/PrintAttributeTokens/PrintAttributeTokens.cpp:48-50
+        if (tokens[i].isLiteral()) {
+          llvm::errs() << "\t=" << tokens[i].getLiteralData();
+        }
----------------
It'd probably be handy to also print identifier tokens.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:236-238
+  /// The number of tokens within the argument.
+  /// The tokens themselves are stored after the object.
+  unsigned NumTokens : 32;
----------------
Rather than use a bit-field for this, I'd add it as a regular `unsigned` after the declaration of `UnavailableLoc`.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:284
+  Token *getTokensBuffer() { return getTrailingObjects<Token>(); }
+  Token const *getTokensBuffer() const { return getTrailingObjects<Token>(); }
+
----------------



================
Comment at: clang/include/clang/Sema/ParsedAttr.h:486
+  const Token *getTokens() const {
+    assert(NumTokens > 0 && "No Tokens to retrieve!");
+    return getTokensBuffer();
----------------
I think it'd be reasonable to return `nullptr` in this case, WDYT?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4100-4102
+  // directly. The recording happens here because this is the only place
+  // where user-defined (via plugins) attributes are parsed, and thus
+  // they care about the token stream directly.
----------------
I think plugins will expect these tokens to be available regardless of which attribute syntax is used, so you may need to look into also doing work for GNU and declspec attribute argument lists.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4121-4122
+    // Add new attribute with the token list.
+    // We assert that we have at least one token,
+    // since we have to ignore the final r_paren.
+    assert(RecordedTokens.size() > 0);
----------------
To be clear, we should have at least *two* tokens, right? One for the left paren and one for the right?


================
Comment at: clang/test/Frontend/plugin-print-attr-tokens.cpp:9-12
+[[plugin::print_tokens("a string")]] void fn1b() {}
+[[plugin::print_tokens()]] void fn1c() {}
+[[plugin::print_tokens(some_ident)]] void fn1d() {}
+[[plugin::print_tokens(int)]] void fn1e() {}
----------------
Can  you also verify that the token stream output from the plugin looks correct?

Can you also add additional tests for GNU and declspec attribute syntaxes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99861