[PATCH] D114790: [syntax][pseudo] Add grammar facilities for the pseudo-parser

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Grammar.h:52
+// Terminal IDs correspond to the clang TokenKind enum.
+using SymbolID = uint16_t;
+// SymbolID is only 12 bits wide.
----------------
sammccall wrote:
> If we want strong types, we could use `enum class SymbolID : uint16_t {};`
we could do that, but I'd leave it as-is at the moment (until we figure out more details about static Grammar construction). 


================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Grammar.h:110
+// Grammar that describes a programming language, C++ etc.
+// It parses BNF grammar rules, and is a building brick for constructing a
+// grammar-based parser.
----------------
sammccall wrote:
> sammccall wrote:
> > nit: building block is more idiomatic
> Nit: it's rather the RuleSpec factories and Grammar::build (which are static) that parses BNF grammar.
> The Grammar object itself doesn't have this responsibility.
Yeah, you're right. Move the BNF-parsing bit to a dedicated place.


================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Grammar.h:144
+
+  // Lookup non-terminal by name.
+  SymbolID lookupSymbol(llvm::StringRef Name) const;
----------------
sammccall wrote:
> The implementation also (inefficiently) supports looking up terminals. Intended?
this is a method function only being used for testing, I don't think we need it. Moved to the test.


================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Grammar.h:171
+
+// Underlying storage of the Grammar.
+struct Table {
----------------
sammccall wrote:
> Are compiled transition tables etc expected to live here, or is this a purely descriptive version of teh grammar?
The later, the Grammar class purely handles grammar symbols/rules. Transition table will be lived somewhere else (table parser), and it will be generated from the TransitionTableBuilder which depends on Grammar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114790