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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
