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