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