sammccall added a comment. (comments for guard part)
================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Language.h:22 + +// Interface for implementing the grammar "guard" attribute. +// ---------------- nit: I think the first sentence should try to describe what this *is* in as simple terms as possible, rather than how it interacts with the rest of the system. ```// A guard restricts when a grammar rule can be used``` ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Language.h:25 +// It is used by the GLR parser to determine whether a reduction of a rule will +// be conducted during the reduce time. +// ---------------- I think it's worth including an example here. "e.g. a guard may allow virt-specifier := IDENTIFIER only if the identifier's text is 'override'". ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Language.h:27 +// +// Returns false if the reduction is not conducted (this parsing branch in GLR +// will die). ---------------- I think this sentence adds more confusion than it helps. (Not sure it's true depending on what you mean by "parsing branch" - the guard runs before the reduce not after, so the branch never exists) ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:306 + auto It = Params.Lang.Guards.find(Rule.Guard); + assert(It != Params.Lang.Guards.end() && "missing guard!"); + if (!It->getSecond()(TempSequence, Tokens)) ---------------- I think diagnosing missing guards but then treating them as always-passing is less surprising and more ergonomic while modifying a grammar Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127448/new/ https://reviews.llvm.org/D127448 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
