[Openmp-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[John Ericson via Phabricator via Openmp-commits]

Ericson2314 added inline comments.


================
Comment at: llvm/CMakeLists.txt:589
     CACHE STRING "Doxygen-generated HTML documentation install directory")
-set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "share/doc/llvm/ocaml-html"
+set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "${CMAKE_INSTALL_DOCDIR}/${project}/ocaml-html"
     CACHE STRING "OCamldoc-generated HTML documentation install directory")
----------------
Ericson2314 wrote:
> compnerd wrote:
> > Why the change from `llvm` -> `${project}`?  (not that it really makes a difference)
> To be honest, I forgot. I might have just been OCD deduping on the fly. Happy to change back,
Which would you like me to do, @compnerd?


================
Comment at: llvm/tools/llvm-config/llvm-config.cpp:361
+    {
+      SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+      sys::fs::make_absolute(ActivePrefix, Path);
----------------
Ericson2314 wrote:
> compnerd wrote:
> > Why the temporary `StringRef`?  Can you not just initialize `Path` with the literal?
> I'm not sure. I would think so too, but the old code was also using `StringRef` so I just followed cargo culted and went with it.
Should I, say, submit a separate patch trying to remove the existing one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810