Hahnfeld added a comment. In https://reviews.llvm.org/D22663#495728, @zlei wrote:
> In https://reviews.llvm.org/D22663#494460, @Hahnfeld wrote: > > > With the changes applied and configured with > > `CLANG_DEFAULT_RTLIB=compiler-rt` some tests fail so you may have to adapt > > the idea of `-rtlib=platform` for the tests. > > > Got it. And I also need to fix those failing tests by adding > `-rtlib=platform` to them, right? Right, I think the tests should pass regardless of what default is selected (which currently is no completely true for `CLANG_DEFAULT_CXX_STDLIB` because it makes `test/Driver/darwin-stdlib.cpp` fail) ================ Comment at: CMakeLists.txt:210 @@ +209,3 @@ + message(WARNING "Resetting default rtlib to use platform default") + set(CLANG_DEFAULT_RTLIB "") +endif() ---------------- zlei wrote: > beanz wrote: > > You'll want this to be: > > > > > > ``` > > set(CLANG_DEFAULT_RTLIB "" CACHE STRING "Default runtime library to use > > (libgcc or compiler-rt)" FORCE) > > ``` > > > > Cached variables can only be overwritten by a `FORCE`. > Sorry, I'm not very familiar with cmake. But why isn't > `CLANG_DEFAULT_CXX_STDLIB` declared with `FORCE`? Should I fix it as well? That's a matter of what we want: without `FORCE` the value will only be reset for that single execution of CMake and not be updated in the cache! This is what I think is correct: The user should be repeatedly reminded that he isn't getting what he originally requested... ================ Comment at: lib/Driver/ToolChain.cpp:530 @@ -538,1 +529,3 @@ + const Arg* A = Args.getLastArg(options::OPT_rtlib_EQ); + StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_RTLIB; ---------------- zlei wrote: > Hahnfeld wrote: > > What I dislike here is that it falls back to the platform default if the > > specified argument value is invalid regardless of what was configured. This > > may be surprising for the user as he gets a different behaviour when > > specifiying `-rtlib=bla` than completely omitting. > > > > I just added a comment explaining how this works in `GetCXXStdlibType`. > > Maybe you can adapt its idea? > If you give an invalid value to `-rtlib`, clang should just abort and give > you an error message, instead of falling back to the platform default. I see > that's also how `-stdlib` behaves. > > Or maybe I misunderstood your point? Ehm yes, you are completely right. For `-rtlib=platform` it should then be enough to write `if (A && LibName != "platform")` to not emit the error in this case. We could later use the same approach to simplify handling of `-stdlib`... https://reviews.llvm.org/D22663 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
