[PATCH] D122303: [pseudo] Sort nonterminals based on their reduction order.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D122303#3402375 <https://reviews.llvm.org/D122303#3402375>, @sammccall wrote:

> The extraction of TestGrammar seems unrelated to the rest of the patch and it *isn't* actually shared between tests yet, which makes it a bit hard to review.

It is used in our previous Forest patch (not landed yet), but you're probably right. I have removed the extraction code in this patch, will send out a followup patch for the extraction afterwards.



================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:55
     };
     for (const auto &Spec : Specs) {
       Consider(Spec.Target);
----------------
sammccall wrote:
> You could identify dependencies here, and then use them to sort the uniquenonterminals before  allocating SymbolIDs
not sure I get the your point -- we could move the topological sort here, but it doesn't seem to be significantly better compared with the current solution.


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:135
+    };
+    std::vector<SymbolID> VisitStates(T->Nonterminals.size(), NotVisited);
+    std::function<void(SymbolID)> DFS = [&](SymbolID SID) -> void {
----------------
sammccall wrote:
> (is a vector of symbol id, or of states?)
oops, it should be the enum State


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122303