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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:147
+      VisitStates[SID] = Visiting;
+      auto It = llvm::partition_point(
+          Dependencies, [&SID](const std::pair<SymbolID, SymbolID> &D) {
----------------
sammccall wrote:
> this is more tersely `llvm::lower_bound(Dependencies, {SID, 0})`, making use of the fact that `std::less` will do what we want
> 
> And with the inline lambda is gone, it seems more idiomatic as a for loop:
> ```
> for (auto It = llvm::lower_bound(...); It != Dependencies.end() && It->first == SID; ++It)
>   DFS(It->second);
> ```
thanks, this is a nice suggestion.


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:156
+      VisitStates[SID] = Visited;
+      Order.push_back(SID);
+    };
----------------
sammccall wrote:
> Instead of building up the permutation array and then inverting it afterwards, maybe directly `Result[SID] = NextKey++;` here?
> 
> (Having pre-sized the vector)
this is a smart suggestion, but it seems too smart, I find the current way is easier to understand (we first build the top order, and revert it secondly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122303