sammccall added inline comments.
================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:12 +# - attributes are omitted, which will be handled as comments; +# - we don't allow nullable non-terminal symbols (except translation-unit). +# There are few nullable non-terminals in the spec grammar, they are adjust ---------------- translation-unit appears to be non-nullable in the grammar here ================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:13 +# - we don't allow nullable non-terminal symbols (except translation-unit). +# There are few nullable non-terminals in the spec grammar, they are adjust +# to be non-nullable; ---------------- nit: adjusted ================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:27 +# FIXME: +# - support annotations (lazy parsing, contextual identifiers) +# - empty input should be parsed successfully (special-case it?) ---------------- I think we should spell out what this is, or leave out the FIXME ================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:28 +# - support annotations (lazy parsing, contextual identifiers) +# - empty input should be parsed successfully (special-case it?) +# ---------------- This probably isn't a FIXME for the grammar right? If we're going with non-nullable rules, having the *parser* special-case _ as nullable seems more sensible. ================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:30 +# +# start symbols +_ := translation-unit ---------------- Is this "default start symbol"? I believe we conceptually want to be able to start parsing at other symbols too. ================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:33 + +# A.1 gram.key +typedef-name := IDENTIFIER ---------------- I'd suggest dropping the A.1 part, as it's version-specific (e.g. gram.basic is currently A.4 at https://eel.is/c++draft/gram, which is a pretty common reference) ================ Comment at: clang/lib/Tooling/Syntax/Pseudo/cxx.bnf:44 + +# A.3 grammar.basic +# !Custom modifications to eliminate optional declaration-seq ---------------- nit: gram.basic ================ Comment at: clang/tools/clang-pseudo/ClangPseudo.cpp:1 +//===-- ClangPseudo.cpp - Clang pseudo parser tool ------------------------===// +// ---------------- Can we use this tool to drive lit tests? ================ Comment at: clang/tools/clang-pseudo/ClangPseudo.cpp:41 + auto Diagnostics = Grammar::build(*RSpecs).diagnose(); + llvm::errs() << (Diagnostics.empty() ? "Parse grammar successfully!\n" + : llvm::join(Diagnostics, "\n")); ---------------- nit: parse -> parsed ================ Comment at: clang/tools/clang-pseudo/ClangPseudo.cpp:42 + llvm::errs() << (Diagnostics.empty() ? "Parse grammar successfully!\n" + : llvm::join(Diagnostics, "\n")); + return 0; ---------------- do you want to dump the grammar? (Behind a dump-grammar subcommand) ================ Comment at: clang/tools/clang-pseudo/ClangPseudo.cpp:43 + : llvm::join(Diagnostics, "\n")); + return 0; +} ---------------- Diagnostics.empty() ? 0 : 2? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115856/new/ https://reviews.llvm.org/D115856 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
