[PATCH] D122270: Support converting pointers from opaque to typed

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/CMakeLists.txt:1
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../)
+
----------------
kuhar wrote:
> kuhar wrote:
> > beanz wrote:
> > > kuhar wrote:
> > > > Could we use `target_include_directories` instead or a global ones?
> > > `include_directories` sets the directory property not the global one, so it only applies to this directory and any subdirectories. There's no reason we couldn't use the target-specific one, but there is no material difference.
> > I see this similar to using global variables in C++. There's no difference *now* but in my experience cmake tends to grow by copy-paste-style development and in X years we may have N targets here all with the same include directories. As we port these to other builds systems like GN or blaze, having clear `target_` includes/libraries/definitions makes life much easier.
> s/blaze/bazel
I'm a lot concerned about this.

On a technical level, `target_include_directories` is used in 3 places in LLVM (all except for one in modules not lists), however `include_directories` is used almost two dozen times many of them throughout the Target subdirectories. The convention is to use `include_directories`. Using directory-based build settings is fundamental to the LLVM build system and has been since the days of the old Makefile build. Anyone translating CMake to another build system needs to understand that and emulate CMake correctly otherwise this will all go off the rails.

I don't think you can draw a parallel between global variables in C++ and directory-scope properties in CMake. They're not the same thing, and our entire build system is built around directory-scoped behaviors which isn't uncommon or unique to CMake. Our old Makefile build had directory-based variable overrides too.

While this may seem like a small change (and should have no material impact), you're asking me to deviate from established conventions in the codebase and I don't believe your reasons justify it. Which leads me to my even bigger concern...

On a policy level, we absolutely should not be considering the impacts on other build systems. The GN and Bazel builds were allowed in-tree on the strict policy that they were optional and would not impact the development or usage of the CMake build. Had anyone suggested in integrating GN or Bazel that we would alter our CMake conventions or usage to accommodate the new build systems they likely wouldn't have been allowed in-tree. The single biggest concern and complaint about adding GN and Bazel was that the community must have _only_ one build system. As the guy who spent 18 months cleaning up CMake so that we could delete the Makefile builds, that's a really important feature.

If, as a policy, we want to move away from directory-based build settings we should have that larger discussion before pushing one patch in a different direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122270