[Lldb-commits] [PATCH] D59708: [ExpressionParser] Add swift-lldb case for finding clang resource dir

[Alex Langford via Phabricator via lldb-commits]

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