[PATCH] D93164: [AST] Add generator for source location introspection

[Nathan James via Phabricator via cfe-commits]

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