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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
