[PATCH] D98774: [AST] De-duplicate empty node introspection

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Why were there two copies in the first place? What was the copy in the cmake file good for, why wasn't the one written by generate_cxx_src_locs.py sufficient? But pulling it into its own file seems nicer than having it inline in python or cmake code.

Another observation: it's a bit strange that the empty impl codepath doesn't share any code with the non-empty code path. The py script could do something like

  if use_empty_implementation:
    jsonData = { classesInClade': [] 'classNames': [] }

and then just fall through to the normal generation code and everything should in theory work with much fewer special cases, right?

(What's a "clade"?)

(I still think the whole approach feels off, as mentioned on the other review.)



================
Comment at: clang/lib/Tooling/CMakeLists.txt:38
+      ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
+      COPYONLY
     )
----------------
Is configure_file(COYPONLY) different from file(COPY)? In what way?

Are you expecting to use cmake vars in the .in file eventually?


================
Comment at: clang/lib/Tooling/CMakeLists.txt:89
       DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
         ${CMAKE_CURRENT_SOURCE_DIR}/DumpTool/generate_cxx_src_locs.py
       COMMAND
----------------
You need to add a dep on the inc file here so that this step reruns if EmptyNodeIntrospection.inc changes.


================
Comment at: clang/lib/Tooling/CMakeLists.txt:96
+        --empty-implementation
+          "${CMAKE_CURRENT_SOURCE_DIR}/EmptyNodeIntrospection.inc.in"
       COMMAND
----------------
What's the advantage of making a copy above? Why not call the checked-in file `EmptyNodeIntrospection.inc` and pass the path to it directly here?


================
Comment at: clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py:176
                   options.output_file), 'w') as f:
-            f.write("""
-namespace clang {
-namespace tooling {
-
-NodeLocationAccessors NodeIntrospection::GetLocations(clang::Stmt const *) {
-  return {};
-}
-NodeLocationAccessors
-NodeIntrospection::GetLocations(clang::DynTypedNode const &) {
-  return {};
-}
-} // namespace tooling
-} // namespace clang
-    """)
+            f.write(options.empty_implementation)
         sys.exit(0)
----------------
Shouldn't this be `f.write(open(options.empty_implementation).read())`?

If you're changing this, you could also make it so that it only writes the output if it'd be different from what it's there, with something like this:


================
Comment at: llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn:8
+    "--use-empty-implementation=1",
+    "--empty-implementation=" + rebase_path(".") + "/EmptyNodeIntrospection.inc",
     "--output-file=" + rebase_path(outputs[0], root_build_dir),
----------------
Don't worry about the gn build, just don't update it at all.

(The input file would have to go into `inputs` for correct incremental builds, the `rebase_path` looks not quite right, there's nothing that copies the .inc.in file to .inc, etc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98774