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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 19 04:50:06 PST 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list