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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
