[PATCH] D128472: [pseudo] Check follow-sets instead of tying reduce actions to lookahead tokens.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 13 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRTable.h:136
+  // check whether a reduction should apply in the current context.
+  llvm::ArrayRef<RuleID> getReduceRules(StateID State) const {
+    return llvm::makeArrayRef(&Reduces[ReduceOffset[State]],
----------------
hokein wrote:
> I think the API is motivated by the LR(0) experimentation, we're now decoupling the reduction look up from the lookahead token, this seems nice (LR(0) only needs `getReduceRules`, other LR(1) variant need `getReduceRules` + `canFollow`).
> 
> The API usage for SLR(1) is not quite straight forward at the first glance,  would be better to add a small snippet in the comment.
Added.

In fact it wasn't motivated by the LR(0) stuff, at least not consciously/directly.
Rather I just wanted to avoid having to allocate a data structure to copy a filtered list into!


================
Comment at: clang-tools-extra/pseudo/lib/grammar/LRTableBuild.cpp:116
+  llvm::DenseMap<StateID, llvm::SmallSet<RuleID, 4>> Reduces;
+  std::vector<llvm::DenseSet<SymbolID>> FollowSets;
+
----------------
hokein wrote:
>  make these members as private -- having two public members seems to violate the pattern. (FollowSets can be initialized in the constructor, we might need a separate method for `Reduces`. or either change the Builder to a struct).
This whole class is private and exists purely for the convenience of buildSLR()/buildForTests, I don't think adding wrapping methods really helps them out.

Changed Builder to a struct and made everything public.


================
Comment at: clang-tools-extra/pseudo/unittests/LRTableTest.cpp:42
+
+  //           eof   semi   term   reduce
+  // +-------+----+-------+------+-------
----------------
hokein wrote:
> the row is usual for terminals,  and the reduce is a separate table, which seems a bit confusing -- I would probably split the reduce table out (adding an empty col between term and reduce)
I moved it to a separate (1D) table above ReduceEntries, and at that point it looked exactly like the definition of ReduceEntries.

So I don't think it adds anything that's not obvious from the code, I dropped the comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128472/new/

https://reviews.llvm.org/D128472