[PATCH] D22663: Support setting default value for -rtlib at build time

[Chris Bieneman via cfe-commits]

beanz added inline comments.

================
Comment at: CMakeLists.txt:210
@@ -202,3 +209,3 @@
 
 set(CLANG_VENDOR ${PACKAGE_VENDOR} CACHE STRING
   "Vendor-specific text for showing with version information.")
----------------
Hahnfeld wrote:
> zlei wrote:
> > Hahnfeld wrote:
> > > zlei wrote:
> > > > I think the original code for resetting `CLANG_DEFAULT_CXX_STDLIB` doesn't work as expected, as beanz pointed out.
> > > > 
> > > > In this revision, I just disable invalid values for both `CLANG_DEFAULT_CXX_STDLIB` and `CLANG_DEFAULT_RTLIB`. Users will get a error message when assigning unsupported values to them.
> > > I tested it this morning and it works as (at least I) expected: It will temporarily reset the value and warn the user that the parameter is not valid.
> > > 
> > > I'm against erroring out here because there actually is a sane default value...
> > I don't have a strong opinion on this, but the problem is the original line `set(CLANG_DEFAULT_CXX_STDLIB "")` doesn't have actual effect (it seems to me).
> > 
> > I'm not sure what you mean by "temporarily reset the value". Last time I tested it, `-DCLANG_DEFAULT_CXX_STDLIB=blah` doesn't reset the value to an empty string (as expected?)
> > 
> > Anyway, I'm fine with either warning or erroring :)
> It means that you will still see `CLANG_DEFAULT_CXX_STDLIB=blah` in `CMakeCache.txt` but it will be temporarily set to `""` when CMake builds the Makefiles.
> 
> Let's have @beanz decide on that: He is the master of CMake and I can only say that it is working as I wanted it to do ;-)
The first `set` call shouldn't be forced because you want to allow users to manually specify the value. If you were to set `FORCE` on it it would always overwrite any value specified on the command line.

The second `set` is debatable one way or the other. The CMake will function correctly with or without the `set` being forced, however as @Hahnfeld commented the invalid value will remain in the CMake cache. That means that every time CMake is re-run the warning will get printed, and the value will be overridden within the scope of that execution.

I think it is better to `FORCE` the second `set` so that the cache gets overwritten with a valid value and the warning only gets logged the first time CMake is run. That said, I don't really have a strong opinion here, so do whatever you think is right.


https://reviews.llvm.org/D22663