sgraenitz marked 7 inline comments as done. sgraenitz added inline comments.
================ Comment at: CMakeLists.txt:43 if(LLDB_INCLUDE_TESTS) + # FIXME: LLDB_TEST_CXX_COMPILER is unused. + # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build directory and not to LLVM's! ---------------- sgraenitz wrote: > labath wrote: > > JDevlieghere wrote: > > > Why is this unused? Are we only using `LLDB_TEST_C_COMPILER` and if so, > > > why? > > I think that's because this only gets passed to dotest, that it has it's > > own logic for deducing the C++ compiler name. > Yes, we only ever pass on `LLDB_TEST_C_COMPILER` and it should be sufficient. > https://github.com/llvm-mirror/lldb/blob/2b03c2512ebf1999e330e85d60b7f93703483485/test/CMakeLists.txt#L53 > > https://lldb.llvm.org/build.html reads: //Note that MSVC is not supported > here, it must be a path to a clang executable.// > As clang == clang++ ⇒ q.e.d :) > > I think what we really want here is a `LLDB_TEST_COMPILER` setting, but this > means: update docs and build bots, potentially phase out the setting? I am > not sure about their popularity. There was a previous initiative for cleanup in D39215. My follow-up patch D56440 aims to finish this. ================ Comment at: CMakeLists.txt:44 + # FIXME: LLDB_TEST_CXX_COMPILER is unused. + # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build directory and not to LLVM's! + # FIXME: LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER options don't work as intended! ---------------- sgraenitz wrote: > JDevlieghere wrote: > > sgraenitz wrote: > > > This was here already when the file was created: > > > https://github.com/llvm-mirror/lldb/blob/98c187f2613fd8e2f5cdf5f8f35503a102a54f11/cmake/modules/LLDBStandalone.cmake#L67 > > > > > > Apparently the below default paths for dsymutil, FileCheck and clang > > > never worked in standalone builds.. > > Instead of this FIXME, should we explicitly unset this variable to prevent > > mistakes in the future? Or is this fixable? > Good point. Not sure what the consequences would be. I will try and see what > happens. See followup patch D56443. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56400/new/ https://reviews.llvm.org/D56400 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
