[PATCH] D127448: [wip][pseudo] Implement guard extension.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 23 06:35:08 PDT 2022
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
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits