[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



More information about the cfe-commits mailing list