[PATCH] D115856: [syntax][pseudo] Introduce the spec C++ grammar.

[Sam McCall via Phabricator via cfe-commits]

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