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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
