njames93 added a comment.
This is almost ready but a few more points need addressing.
Running clang-format over the inc file is pointless and just extends
compilation time while adding an unnecessary dependency on clang-format.
The inc file should likely live in the include build directory, All tablegen
files seem to live in there. You could either move the CMake code that
generates it into the include directory, or alter the directory, this should do
that if you want and its safer than replace as it would only change the last
/lib/ detected.
# Replace the last lib component of the current binary directory with include
string(FIND ${CMAKE_CURRENT_BINARY_DIR} "/lib/" PATH_LIB_START REVERSE)
if(PATH_LIB_START EQUAL -1)
message(FATAL_ERROR "Couldn't find lib component in binary directory")
endif()
math(EXPR PATH_LIB_END "${PATH_LIB_START}+5")
string(SUBSTRING ${CMAKE_CURRENT_BINARY_DIR} 0 ${PATH_LIB_START} PATH_HEAD)
string(SUBSTRING ${CMAKE_CURRENT_BINARY_DIR} ${PATH_LIB_END} -1 PATH_TAIL)
string(CONCAT BINARY_INCLUDE_DIR ${PATH_HEAD} "/include/" ${PATH_TAIL})
After moving it to the Include output folder, In the cpp file you would need
`#include "clang/Tooling/NodeIntrospection.inc"`.
This would also remove a lot of those commands in `lib/tooling/CMakeLists.txt`.
Tablegen has a command line option `--write-if-changed` It may be wise to also
include that in your generator script instead of using copy-if-different in the
aforementioned CMakeLists.txt.
================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:16-17
+
+#include <clang/AST/ASTTypeTraits.h>
+#include <clang/AST/DeclarationName.h>
+
----------------
These should be quoted includes
================
Comment at: clang/lib/Tooling/CMakeLists.txt:29
+
+add_custom_command(
+ OUTPUT ${CMAKE_BINARY_DIR}/ASTNodeAPI.json
----------------
It may be wise to use the `COMMENT` argument to let the users know that it's
building the ASTNodeAPI.json.
================
Comment at: clang/lib/Tooling/CMakeLists.txt:56
+ COMMAND
+ ${PYTHON_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/DumpTool/generate_cxx_src_locs.py
+ --json-input-path ${CMAKE_BINARY_DIR}/ASTNodeAPI.json
----------------
Likewise a comment to say building NodeIntrospection.inc.
================
Comment at: clang/lib/Tooling/CMakeLists.txt:99
+ NodeIntrospection.cpp
+ NodeIntrospection.inc
Tooling.cpp
----------------
This shouldn't appear in the source list.
================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:10
+
+#ifndef LLVM_CLANG_TOOLING_APIDATA_H
+#define LLVM_CLANG_TOOLING_APIDATA_H
----------------
aaron.ballman wrote:
> Might as well fix this lint warning.
Header guard should be `LLVM_CLANG_LIB_TOOLING_DUMPTOOL_APIDATA_H`
================
Comment at: clang/lib/Tooling/NodeIntrospection.cpp:13-15
+#include <clang/AST/AST.h>
+
+#include <clang/Tooling/NodeIntrospection.h>
----------------
Quoted includes and the `NodeIntrospection.h` include is the MainFileInclude so
should appear first.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93164/new/
https://reviews.llvm.org/D93164
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
