[PATCH] D126536: [pseudo] Add grammar annotations support.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h:22
 //
+//  The grammar supports extensions, which have the syntax form of
+//  [key=value;key=value]. Extensions are associated with a grammar symbol (
----------------
sammccall wrote:
> Hmm, I think we misunderstood each other...
> 
> I think annotations is a good name for the syntactic [foo=bar] bit. I think the comment here is now harder to follow, and would prefer the old version.
> 
> It's specifically "bar" which indicates some identifier whose semantics will be provided externally, so "bar" is a reference to an extension.
> 
> If you want to mention the concept of an extension, then maybe at the end...
> `Each unique annotation value creates an extension point. The Override guard is implemented in CXX.cpp by binding the ExtensionID for Override to a C++ function that performs the check.`
oops, I misunderstood your "extensionID" comment completely (sorry). Bring it back now.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h:213
   std::vector<Nonterminal> Nonterminals;
+  // A table of extensions values, sorted by name.
+  // ExtensionID is the index of the table.
----------------
sammccall wrote:
> attribute values, or extension names
> 
> (attributes are syntactic and have string values, extensions are semantic and can be referred to by names).
renamed to attribute values.


================
Comment at: clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:246
+      auto KV = ExtText.split('=');
+      if (KV.first == ExtText) { // no separator in Line
+        Diagnostics.push_back(
----------------
sammccall wrote:
> If we drop this line, then [foo] will be equivalent to [foo=], i.e. attribute is present and points at the empty string. This seems reasonable to me, and is a good syntax for boolean attributes (where [foo] denotes set and no attribute denotes unset)
This syntax for bool-type attributes seems good to me.
Note that there is a subtle difference with the string-type attributes, where we use the empty string as the sentinel attribute value (denote unset) whose ExtensionID is 0. I think it should not be a big issue (we only handle them is in the bnf parsing time, and propagate field values in `Rule`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126536