Page MenuHomePhabricator

[cmake] Guard another instance where symlinks are being created
Needs ReviewPublic

Authored by azharudd on May 22 2018, 1:36 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

azharudd created this revision.May 22 2018, 1:36 PM

I'm not sure about this patch: These commands do not install the aliases, they are created in the build directory and may be needed for testing.

(And shouldn't this say if (LIBOMP_INSTALL_ALIASES), according to your change in D47221?)

I'm not sure about this patch: These commands do not install the aliases, they are created in the build directory and may be needed for testing.

This should be NFC as by default that flag would be ON. I see it being used in lines 306-314 which is dealing with installing the symlinks. Maybe this is just duplicate code.

(And shouldn't this say if (LIBOMP_INSTALL_ALIASES), according to your change in D47221?)

I just reused the existing if condition in this file and wanted to remain consistent. At least it doesn't cause a cmake error in this case as the if condition would still be correct.

I'm not sure about this patch: These commands do not install the aliases, they are created in the build directory and may be needed for testing.

This should be NFC as by default that flag would be ON. I see it being used in lines 306-314 which is dealing with installing the symlinks. Maybe this is just duplicate code.

It changes things if you set a non-default patch, so it's definitely not NFC, no matter what the default is.

(And shouldn't this say if (LIBOMP_INSTALL_ALIASES), according to your change in D47221?)

I just reused the existing if condition in this file and wanted to remain consistent. At least it doesn't cause a cmake error in this case as the if condition would still be correct.

You made the point that the existing conditions are wrong, so I don't think we should add new code that doesn't follow the CMake documentation.