[PATCH] D72875: [clang][cmake] Include generated rst files in html built by docs-clang-html target

[Dan Liew via Phabricator via llvm-commits]

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