[PATCH] D93164: [AST] Add generator for source location introspection

Sun Mar 14 06:44:39 PDT 2021

thakis added a subscriber: rsmith.
thakis added a comment.

A few more high-level questions:

- What's the point of the intermediary json file? Why not generate the final c++ directly? (As far as I can tell, this wasn't discussed during the review yet)
- Do we need to generate code for this at all? Could this be done via xmacros or tablegen?

Having a bespoke custom python -> json -> python -> c++ pipeline here seems like it's fairly different from how the rest of clang does things, and it seems like it duplicates some of the existing tooling we have here.

(Having said that, I'm no code owner here -- @rsmith is. Maybe he has an opinion.)

Lower-level: Did you see all the comments on https://reviews.llvm.org/rGd627a27d264b47eda3f15f086ff419dfe053ebf7 ? This relanded with them unaddressed. Please address them in a follow-up. (Sorry for leaving the comments on the commit instead of the review!)

================
Comment at: clang/lib/Tooling/CMakeLists.txt:31
+    COMMENT Generate ASTNodeAPI.json
+    OUTPUT ${CMAKE_BINARY_DIR}/ASTNodeAPI.json
+    DEPENDS clang-ast-dump clang-headers
----------------
Putting this in the root of the build dir seems a bit untidy. I think `CMAKE_CURRENT_BINARY_DIR` is what we usually use for generated files.

================
Comment at: clang/lib/Tooling/CMakeLists.txt:74
+    ${CMAKE_COMMAND} -E copy_if_different
+      ${CMAKE_CURRENT_BINARY_DIR}/generated/NodeIntrospection.inc
+      ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
----------------
...like used here. Why `generated/`? Everything in CMAKE_CURRENT_BINARY_DIR is generated. (Compare to `find . -name '*.inc'`)

Also, why not make the python script write the file only if changed instead of making a copy here? (like llvm-tblgen does)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93164