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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
