sammccall added a comment. In D122303#3402375 <https://reviews.llvm.org/D122303#3402375>, @sammccall wrote:
> I think my main suggestion is to sort the symbols, not just the rules. > Having the rules in blocks grouped by the symbol they produce, but not in > symbol order, seems confusing to reason about. We discussed this offline - having alphabetical symbol IDs is certainly nice, and there's no hard technical reason to give rules and symbols the same order. I've come around to the (surprising!) approach in this patch, and just think we should document it so it's less surprising. ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h:168 - // The rules are sorted (and thus grouped) by target symbol. + // The rules are topological sorted (and thus grouped) by the target symbol. + // ---------------- sammccall wrote: > I think it's easier to understand if we sort the *symbols* and leave this > comment as-is. > It feels a bit strange to sort the symbols alphabetically but sort the rules > by some other property of the symbols. > > It's also important to describe what relation we're sorting by! > > I'd say "The symbols are topologically sorted: if `S := T` then S has a > higher SymbolID than T." Revised suggestion: ``` RuleID is an index into this array of rule definitions. Rules with the same target symbol (LHS) are grouped into a single range. The relative order of different target symbols is *not* by SymbolID, but rather a topological sort: if S := T then the rules producing T have lower RuleIDs than rules producing S. (This strange order simplifies the LR parser: for a given token range, if we reduce in increasing RuleID order then we need never backtrack - prerequisite reductions are reached before dependent ones). 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
