[AST] De-duplicate empty node introspection

[steveire]

Summary

This way we can add support for other nodes without duplication.

[thakis]

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.)

clang/lib/Tooling/CMakeLists.txt

38

Is configure_file(COYPONLY) different from file(COPY)? In what way?

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

89

You need to add a dep on the inc file here so that this step reruns if EmptyNodeIntrospection.inc changes.

96

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?

clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py

176

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:

llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn

8

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)

[steveire]

Update

[steveire]

In D98774#2631898, @thakis wrote:

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?

AFAIK python is not a hard-requirement of the llvm/clang build.

But pulling it into its own file seems nicer than having it inline in python or cmake code.

Yes.

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?

I don't think this cuts down on special cases. Also, it looks like it would generate code with usings which are never used

using LocationAndString = SourceLocationMap::value_type;
using RangeAndString = SourceRangeMap::value_type;

which would lead us to having to handle "unused using" warnings.

Copying the file seems a good compromise between the options.

(What's a "clade"?)

The group of types which have a common ancestor. Everything inheriting from Stmt is in a different clade to everything inheriting from Decl.

clang/lib/Tooling/CMakeLists.txt

38

configure_file only copies the file if it is different. file(COPY) does too, but it doesn't seem to include a way to specify the destination file name.

96

I'm not sure what you mean. The configure_file is inside the if and this line is inside the else. Does that clear it up?

[steveire]

@thakis Do you have any more on this? Can we de-duplicate?

[thakis]

Definitely a step forward, so lg :)

AFAIK python is not a hard-requirement of the llvm/clang build.

I think it is? https://llvm.org/docs/GettingStarted.html#software lists it at least.

clang/lib/Tooling/CMakeLists.txt

96

Oh, I see. Maybe we could always call the py script and only make it copy through the empty file if the script isn't supposed to do anything?

[steveire]

In D98774#2701853, @thakis wrote:

I think it is? https://llvm.org/docs/GettingStarted.html#software lists it at least.

Thanks, I'll look into that soon and see how the buildbots respond.

[thakis]

In D98774#2708108, @steveire wrote:

In D98774#2701853, @thakis wrote:

I think it is? https://llvm.org/docs/GettingStarted.html#software lists it at least.

Thanks, I'll look into that soon and see how the buildbots respond.

I looked into a bit too, and at least gen_decision_forest in clang-tools-extra uses python without a guard. See clang-tools-extra/clangd/quality/CompletionModel.cmake and clang-tools-extra/clangd/CMakeLists.txt

clang/lib/Tooling/CMakeLists.txt

 
 if (NOT Python3_EXECUTABLE
     OR WIN32
     OR APPLE
     OR GENERATOR_IS_MULTI_CONFIG
     OR NOT LLVM_NATIVE_ARCH IN_LIST LLVM_TARGETS_TO_BUILD
     OR NOT X86 IN_LIST LLVM_TARGETS_TO_BUILD
     )
-  file(GENERATE OUTPUT ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
-    CONTENT "
-namespace clang {
-namespace tooling {
-
-NodeLocationAccessors NodeIntrospection::GetLocations(clang::Stmt const *) {
-  return {};
-}
-NodeLocationAccessors
-NodeIntrospection::GetLocations(clang::DynTypedNode const &) {
-  return {};
-}
-} // namespace tooling
-} // namespace clang
-"
+    configure_file(
+      EmptyNodeIntrospection.inc.in
+      ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
+      COPYONLY
     )
     set(CLANG_TOOLING_BUILD_AST_INTROSPECTION "OFF" CACHE BOOL "")
 else()
   # The generation of ASTNodeAPI.json takes a long time in a
   # Debug build due to parsing AST.h. Disable the processing
   # but setting CLANG_TOOLING_BUILD_AST_INTROSPECTION as an
   # internal hidden setting to override.
   # When the processing is disabled, a trivial/empty JSON
")
   add_custom_target(run-ast-api-dump-tool
       DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
   )
 
   add_custom_command(
       COMMENT Generate NodeIntrospection.inc
       OUTPUT ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
       DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
         ${CMAKE_CURRENT_SOURCE_DIR}/DumpTool/generate_cxx_src_locs.py
       COMMAND
       ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/DumpTool/generate_cxx_src_locs.py
         --json-input-path ${CMAKE_CURRENT_BINARY_DIR}/ASTNodeAPI.json
         --output-file NodeIntrospection.inc
-        --empty-implementation ${skip_expensive_processing}
+        --use-empty-implementation ${skip_expensive_processing}
+        --empty-implementation
+          "${CMAKE_CURRENT_SOURCE_DIR}/EmptyNodeIntrospection.inc.in"
       COMMAND
       ${CMAKE_COMMAND} -E copy_if_different
         ${CMAKE_CURRENT_BINARY_DIR}/NodeIntrospection.inc
         ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
   )
 
   add_custom_target(run-ast-api-generate-tool
       DEPENDS

clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py

 
 def main():
 
     parser = argparse.ArgumentParser()
     parser.add_argument('--json-input-path',
                       help='Read API description from FILE', metavar='FILE')
     parser.add_argument('--output-file', help='Generate output in FILEPATH',
                       metavar='FILEPATH')
-    parser.add_argument('--empty-implementation',
+    parser.add_argument('--use-empty-implementation',
                       help='Generate empty implementation',
                       action="store", type=int)
+    parser.add_argument('--empty-implementation',
+                      help='Copy empty implementation from FILEPATH',
+                      action="store", metavar='FILEPATH')
 
     options = parser.parse_args()
 
-    use_empty_implementation = options.empty_implementation
+    use_empty_implementation = options.use_empty_implementation
 
     if (not use_empty_implementation
             and not os.path.exists(options.json_input_path)):
         use_empty_implementation = True
 
     if not use_empty_implementation:
         with open(options.json_input_path) as f:
             jsonData = json.load(f)
 
         if not 'classesInClade' in jsonData or not jsonData["classesInClade"]:
             use_empty_implementation = True
 
     if use_empty_implementation:
         with open(os.path.join(os.getcwd(),
                   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)
 
     g = Generator()
 
     g.GeneratePrologue()
 
     for (CladeName, ClassNameData) in jsonData['classesInClade'].items():
         g.GenerateBaseGetLocationsDeclaration(CladeName)

clang/lib/Tooling/EmptyNodeIntrospection.inc.in

+//===- EmptyNodeIntrospection.inc.in --------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+namespace clang {
+namespace tooling {
+
+NodeLocationAccessors NodeIntrospection::GetLocations(clang::Stmt const *) {
+  return {};
+}
+NodeLocationAccessors
+NodeIntrospection::GetLocations(clang::DynTypedNode const &) {
+  return {};
+}
+} // namespace tooling
+} // namespace clang

llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn

 # FIXME: The cmake build runs DumpTool:clang-ast-dump to generate a json
 # file and feeds it into this step in non-debug builds or if an option is set.
 action("node_introspection_inc") {
   script = "DumpTool/generate_cxx_src_locs.py"
   outputs = [ "$target_gen_dir/clang/Tooling/NodeIntrospection.inc" ]
   args = [
-    "--empty-implementation=1",
+    "--use-empty-implementation=1",
+    "--empty-implementation=" + rebase_path(".") + "/EmptyNodeIntrospection.inc",
     "--output-file=" + rebase_path(outputs[0], root_build_dir),
   ]
 }
 
 static_library("Tooling") {
   output_name = "clangTooling"
   configs += [ "//llvm/utils/gn/build:clang_code" ]
   deps = [