[PATCH] D127448: [wip][pseudo] Implement guard extension.

[Sam McCall via Phabricator via cfe-commits]

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