[PATCH] D125667: [pseudo] A basic implementation of compiling cxx grammar at build time.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Few initial comments...



================
Comment at: clang-tools-extra/pseudo/gen/Main.cpp:8
+//===----------------------------------------------------------------------===//
+
+#include "clang-pseudo/Grammar.h"
----------------
missing description


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cxx/cxx.h:35
+// It provides a simple uniform way to access a particular nonterminal.
+enum Symbol : SymbolID {
+#define NONTERMINAL(X, Y) X = Y,
----------------
enum class? I suspect having to cast these explicitly is worth it if using them is rare.

Otherwise we'll end up with rules in the same namespace as symbols, later.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cxx/cxx.h:37
+#define NONTERMINAL(X, Y) X = Y,
+#include "CxxSymbols.inc"
+#undef NONTERMINAL
----------------
we've got {cxx, Cxx} in filenames.
We should be consistent, and per LLVM style I think `CXX` is correct?


================
Comment at: clang-tools-extra/pseudo/lib/CMakeLists.txt:3
 
-add_clang_library(clangPseudo
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES
----------------
We do have a layering relationship here, and a requirement to keep the "grammar" dependencies small - should we move it into a subdirectory?


================
Comment at: clang-tools-extra/pseudo/lib/CMakeLists.txt:45
+include(${CMAKE_CURRENT_SOURCE_DIR}/../gen/cxx_gen.cmake)
+add_clang_library(clangPseudoCxx
+  cxx/cxx.cpp
----------------
why is this not in cxx/CMakeLists.txt?


================
Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.cpp:1
+//===--- cxx.cpp - Define public intefaces for C++ grammar ----------------===//
+//
----------------
nit: interfaces


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125667/new/

https://reviews.llvm.org/D125667