daltenty wrote: > Uhm - this looks pretty big and seems like something that can easily break > certain build configurations since it doesn't seem to touch only AIX
Agreed that this looks big and scary, but it's a purely mechanical change, that is a no-op for most targets. I'll add a long form rational at the end of the comment about why I don't think the patch effects anyone but AIX to keep my answers brief. >Is this in main without any issues? Yes, these patches have been in main for several weeks at this point with no reported issues. > Does it really NEED to be merged for the release branch at this point? It would help us out for the point releases. Without this patch, we're unable to build on AIX with CMake from our package manager (4.0). We can manually downgrade if we're unwilling **Rationale about why the patch doesn't affect targets besides AIX** We quote the string AIX and variable expansions which might expand to string AIX (i.e. `CMAKE_SYSTEM_NAME`), so that we do the intent string comparison. If not quoted the if will expand the string if it happens to match a variable name (which `AIX` does in CMake 4.0+). This has an effect only if `CMAKE_SYSTEM_NAME` (https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html) expands to something which is a CMake variable (https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html#variables-that-describe-the-system) Intersecting the two list gives me the following list of affect targets: ``` AIX CYGWIN MSYS WASI ``` Of those targets, only CYGWIN appears in the lines affected by the patch, and it's already using a variable check (i.e. it checks `CYGWIN`) not a string comparison to `CMAKE_SYSTEM_NAME`, so it's unaffected. https://github.com/llvm/llvm-project/pull/156505 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
