sammccall added a comment. The ParseLang + CLI library seem clearly separate from everything else, I think this patch should be split.
Comments on that part first... ================ Comment at: clang-tools-extra/pseudo/benchmarks/Benchmark.cpp:68 for (auto _ : State) - LRTable::buildSLR(*G); + LRTable::buildSLR(cli::getLanguage().G); } ---------------- getLanguage() should not be inside the loop. This is happening in a few places Maybe it should stay in setupGrammarAndSource? ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Language.h:35 +struct ParseLang { + const Grammar &G; + const LRTable &Table; ---------------- why are these all references with ParseLang (presumably) passed by value, instead of values with ParseLang passed by reference? ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:24 +struct ParseLang; +namespace cli { +// Returns the corresponding language from the '--grammar' command-line flag. ---------------- nit: I don't think a namespace is needed here, compared to just `getLanguageFromFlags()` ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:26 +// Returns the corresponding language from the '--grammar' command-line flag. +const ParseLang &getLanguage(); +} // namespace cli ---------------- either "language" is a clear enough name or it isn't - given naming elsewhere this should be getParseLang I think ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:298 auto Pop = [&](const GSS::Node *Head, RuleID RID) { - LLVM_DEBUG(llvm::dbgs() << " Pop " << Params.G.dumpRule(RID) << "\n"); - const auto &Rule = Params.G.lookupRule(RID); + LLVM_DEBUG(llvm::dbgs() << " Pop " << Params.Lang.G.dumpRule(RID) << "\n"); + const auto &Rule = Params.Lang.G.lookupRule(RID); ---------------- I'm not sure exactly what the best thing to do about this is, but `Params.Lang.G` is hard to read - it's a long complicated name that doesn't even name the thing. I think probably we should split ParseParams up instead of nesting ParseLang in it, e.g. ``` struct GLRStorage { ForestArena&, GSS } glrReduce(vector<ParseStep>, const ParseLang&, GLStorage&, NewHeadCallback) ``` Or even put a forest pointer in the GSS just for convenience. ================ Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:52 + llvm::errs() << Diag << "\n"; + std::exit(1); + } ---------------- huh? diagnostics aren't fatal (at least haven't been considered so before) ================ Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:53 + std::exit(1); + } + ---------------- "missing guard function" is probably a grammar diagnostic, maybe we should add some logic to validate extensions? Probably a FIXME for now ================ Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:57-58 + llvm::DenseMap<ExtensionID, Guard> *Guards = + new llvm::DenseMap<ExtensionID, Guard>(); + for (ExtensionID ID = 1; ID < G->table().AttributeValues.size(); ++ID) + Guards->insert(std::make_pair(ID, alwaysAccept)); ---------------- I don't think the CLI library should be in charge of knowing the fallback logic for each type of extension. Could the parser handle a missing guard itself instead? ================ Comment at: clang-tools-extra/pseudo/tool/ClangPseudo.cpp:94 - if (Grammar.getNumOccurrences()) { - std::string Text = readOrDie(Grammar); - std::vector<std::string> Diags; - auto G = Grammar::parseBNF(Text, Diags); - - if (!Diags.empty()) { - llvm::errs() << llvm::join(Diags, "\n"); - return 2; - } - llvm::outs() << llvm::formatv("grammar file {0} is parsed successfully\n", - Grammar); + if (true) { + const auto &Lang = clang::pseudo::cli::getLanguage(); ---------------- drop if(true)? ================ Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:51 } - + // FIXME: move to TokenStream class. + TokenStream emptyTokenStream() { ---------------- either do it or don't - this is too small to check in a fixme IMO ================ Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:57 + } + ParseLang getTestLang() { + return {*G, Table, Guards}; ---------------- can this just be a member? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127448/new/ https://reviews.llvm.org/D127448 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits