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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to