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

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked 4 inline comments as done.
steveire added a comment.

In D98774#2631898 <https://reviews.llvm.org/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 `using`s 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`.



================
Comment at: clang/lib/Tooling/CMakeLists.txt:38
+      ${BINARY_INCLUDE_DIR}/NodeIntrospection.inc
+      COPYONLY
     )
----------------
thakis wrote:
> Is configure_file(COYPONLY) different from file(COPY)? In what way?
> 
> Are you expecting to use cmake vars in the .in file eventually?
`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.


================
Comment at: clang/lib/Tooling/CMakeLists.txt:96
+        --empty-implementation
+          "${CMAKE_CURRENT_SOURCE_DIR}/EmptyNodeIntrospection.inc.in"
       COMMAND
----------------
thakis wrote:
> 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?
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98774