xiaobai added a comment.
In D59708#1439872 <https://reviews.llvm.org/D59708#1439872>, @jingham wrote:
> The behavior when verify is false seems a little weird to me. In that case
> you will just always get the $install_dir/lib/clang/$clang_version path.
> That's fairly different from the verify = true case. OTOH, I couldn't see
> anybody passing verify = false anywhere. Do we need that?
The only use of `verify = false` is in the ClangHostTest (in the poorly named
`unittests/Expression/ClangParserTest.cpp`). We pass a dummy path with verify
set to false in order to make sure that it can correctly append
`lib/clang/$clang_version`. Then we pass verify = true and make sure that it
doesn't actually exist.
================
Comment at: source/Plugins/ExpressionParser/Clang/ClangHost.cpp:67
+ relative_path.clear();
+ llvm::sys::path::append(relative_path, "lib", "lldb", "clang");
+ llvm::sys::path::append(clang_dir, relative_path);
----------------
compnerd wrote:
> Does swift-lldb never honour `LIBDIR`? That is, can it never end up in
> `lib64`?
Oh, it absolutely does. I'll fix that!
================
Comment at: source/Plugins/ExpressionParser/Clang/ClangHost.cpp:73
+ return true;
+ }
+
----------------
compnerd wrote:
> I think it would be nicer if you could create a static list of the paths and
> loop over it:
>
> ```
> static const StringRef kResourceDirSuffixes[] = {
> "lib" TO_STRING(CLANG_LIBDIR_SUFFIX) "/clang" CLANG_VERSION_STRING,
> "lib" TO_STRING(CLANG_LIBDIR_SUFFIX) "/lldb/clang",
> };
>
> for (const auto &Suffix : kResourceDirSuffixes) {
> llvm::SmallString<256> clang_dir(parent_dir);
> llvm::SmallString<32> relative_path(Suffix);
> llvm::sys::path::native(relative_path);
> llvm::sys::path::append(clang_dir, relative_path);
> if (!verify || VerifyClangPath(clang_dir)) {
> file_spec.GetDirectory().SetString(clang_dir);
> FileSystem::Instance().Resolve(file_spec);
> return true;
> }
> }
> ```
Yeah sounds good to me. Seems more manageable.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59708/new/
https://reviews.llvm.org/D59708
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
