delcypher requested changes to this revision.
delcypher added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/docs/CMakeLists.txt:93
+function (gen_rst_file output_file td_option source)
+ get_filename_component(TABLEGEN_INCLUDE_DIR
"${CMAKE_CURRENT_SOURCE_DIR}/${source}" DIRECTORY)
----------------
`gen_rst_file` is a very ambiguous name. At call sites it might not be obvious
what this function does. Something like `gen_rst_file_from_td` might be more
helpful.
================
Comment at: clang/docs/CMakeLists.txt:94
+function (gen_rst_file output_file td_option source)
+ get_filename_component(TABLEGEN_INCLUDE_DIR
"${CMAKE_CURRENT_SOURCE_DIR}/${source}" DIRECTORY)
+ list(APPEND LLVM_TABLEGEN_FLAGS "-I${TABLEGEN_INCLUDE_DIR}")
----------------
You might want to add a check that `${CMAKE_CURRENT_SOURCE_DIR}/${source}`
exists in this function and error if it does not exist.
================
Comment at: clang/docs/CMakeLists.txt:97
+ clang_tablegen(${output_file} ${td_option} SOURCE ${source} TARGET
"gen-${output_file}")
+ add_dependencies(docs-clang-html "gen-${output_file}")
+endfunction()
----------------
This `add_dependencies(...)` command is very fragile and is creating an
implicit coupling between:
* The implementation of `add_sphinx_target()`.
* The declaration of the clang html sphinx target.
* The implementation of `gen_rst_file`.
`gen_rst_file` should probably take an optional argument for the target (in
this case `docs-clang-html`) to add a dependency to.
This does not completely fix the dependency on `add_sphinx_target`
implementation but it does mean that `gen_rst_file` isn't also dependent.
================
Comment at: clang/docs/CMakeLists.txt:104
if (${SPHINX_OUTPUT_HTML})
- add_sphinx_target(html clang)
+ add_sphinx_target(html clang SOURCE_DIR ${CMAKE_CURRENT_BINARY_DIR})
+
----------------
Minor nit: You might want to quote uses of `${CMAKE_CURRENT_BINARY_DIR}`,
`${CMAKE_COMMAND}`, and `${CMAKE_CURRENT_SOURCE_DIR}`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72875/new/
https://reviews.llvm.org/D72875
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
