sammccall added a comment. Thanks, the split here looks good!
================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:115 // Parameters for the GLR parsing. +// FIXME: refine it with the ParseLang struct. struct ParseParams { ---------------- You're already touching the callsites of the glr* functions, it might be worth just doing this already... up to you ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:1 +//===--- ParseLang.h ------------------------------------------- -*- C++-*-===// +// ---------------- The "ParseLang" name doesn't feel right to me :-( I think it's a combination of: - Lang is unneccesarily abbreviated - "Parse" doesn't actually disambiguate much, as "parse" is the whole project Do you think `clang::pseudo::Language` would work? (Sorry for not realizing this on the previous pass, I know it's a pain... happy to chat more offline) ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:19 +// Specify a language that can be parsed by the pseduoparser. +// Manifest generated from a bnf grammar file. +struct ParseLang { ---------------- I don't know what this sentence means - is it always true? is it necessary? ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:24 + + // FIXME: add clang::LangOptions. + // FIXME: add default start symbols. ---------------- is this "fix right after this patch" or "fix sometime, maybe" or something in between? (I think these make a lot of sense, and am worried the structure will be incoherent if we don't actually add them soon) ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:9 +// +// A library shared among different pseudoparser-based tools. It provides a +// uniform way to get basic pieces of the parser (Grammar, LRTable etc) from ---------------- The first sentence should say what it is, rather than what calls it. If the file by design defines a single thing function I'm not sure a file comment distinct from the function comment helps much, maybe merge them? ================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:27 +// Returns the corresponding language from the '--grammar' command-line flag. +const ParseLang &getParseLangFromFlags(); + ---------------- this should generally be called exactly one from CLI tools - why does it need to be memoized and returned as a reference? ================ Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:38 + << "': " << EC.message() << "\n"; + std::exit(1); + } ---------------- this is extremely surprising. At minimum it needs to be very clearly documented, but I think it's probably better to return null to the caller ================ Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:42 + auto G = Grammar::parseBNF(GrammarText->get()->getBuffer(), Diags); + if (!Diags.empty()) { + for (const auto &Diag : Diags) ---------------- this if() isn't needed unless you want to print a header to provide some context (which might be a good idea) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128679/new/ https://reviews.llvm.org/D128679 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits