[PATCH] D120788: [cmake] Add INTERFACE_INCLUDE_DIRECTORIES to LLVM and MLIR.

[Stella Laurenzo via Phabricator via llvm-commits]

stellaraccident added inline comments.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:821
+    $<BUILD_INTERFACE:${LLVM_INCLUDE_DIR}>
+  )
+
----------------
marbre wrote:
> With your change to `llvm_add_library()` the generated objlib //can// propagate "public" includes, as `PUBLIC` in setting `INCLUDE_DIRECTORIES` and  `INTERFACE_INCLUDE_DIRECTORIES` (which looks good to me 👍).
> 
> Here, the public includes are added after the calling `llvm_add_library()`. A rapidly prototyped test script lets me assume that the order might important. Should the public includes be added before calling `llvm_add_library()` to mirror those?
> Here, the public includes are added after the calling llvm_add_library(). A rapidly prototyped test script lets me assume that the order might important. Should the public includes be added before calling llvm_add_library() to mirror those?

Can you say more -- I'm not quite sure I'm following? Iiuc, the existing include mechanism depends on bootstrapping from the directory INCLUDE_DIRECTORIES property (which is a vestige of legacy interop). There is not such a mechanism for INTERFACE_INCLUDE_DIRECTORIES, so it does need to be after library creation. But I'm not seeing a more subtle order dependence (not saying there isn't one, just not seeing it).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120788/new/

https://reviews.llvm.org/D120788