[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Updated according to your comments.



================
Comment at: lib/Sema/SemaDeclAttr.cpp:3481
+  llvm::StringMap<int> NameIdxMapping;
+  NameIdxMapping["__"] = -1;
+
----------------
aaron.ballman wrote:
> This doesn't match the documentation -- I assume you switched from `?` to `__` because `__` at least parses as a valid identifier, whereas `?` would require extra parsing support? If so, that's fine by me.
Yes, `__`, and `__this` where chosen because they work without lexer/parser changes and are in the implementation namespace. I forgot to update the documentation though. Will be fixed.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3483
+
+  NameIdxMapping["__this"] = 0;
+
----------------
aaron.ballman wrote:
> This doesn't match the documentation either, but I'm less clear on why the double underscores are used.
If you use `this`, the lexer will generate the special "this" token. That one is checked explicitly to be only used inside of non-static class methods. If you have an idea how to avoid this check or make it consider uses in the attribute as OK, please let me know.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3492
+  SmallVector<int, 8> EncodingIndices;
+  for (unsigned u = 0, e = AL.getNumArgs(); u < e; u++) {
+
----------------
aaron.ballman wrote:
> Identifiers don't match the usual naming conventions.  Prefer `++U` as well.
OK.


> Prefer ++U as well.

Out of curiosity, why?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3493
+  for (unsigned u = 0, e = AL.getNumArgs(); u < e; u++) {
+
+    SourceRange SR;
----------------
aaron.ballman wrote:
> Spurious newline
That was intentional but OK.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55483