[Lldb-commits] [PATCH] D47384: Remove dependency from Host to clang

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: aprantl.
labath added a comment.

+Adrian.

I remember looking at the path computation code a while ago (I think it was in the context of python though) and concluding that something like this would be needed. Overall the patch seems fine to me, but I want to make sure it gets enough visibility, so I've added Adrian as he was working on this code recently.

I think that a nice cleanup here would be to replace the private `HostInfoBase::GetLLDBPath` function with a sequence of functions for accessing individual path types (so basically do `s/HostInfo::GetLLDBPath(ePathTypeXXX/HostInfo::GetXXXPath(/`). All the private users of this function are calling it with a fixed enum value anyway, so this would be a slight simplification on their side anyway. Then the only place dealing with this enum would be the SBHostOS class, which could to the dispatch on the full set of enum values. This would allow us to avoid having a function which does not make sense for certain enum values (which is even more important, as sooner or later, we will need to do a similar patch for `ePathTypePythonDir`).



================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.h:17-20
+#if defined(__APPLE__)
+bool ComputeClangDirectory(FileSpec &lldb_shlib_spec, FileSpec &file_spec,
+                           bool verify);
+#endif
----------------
If I understand this correctly, this function is only in the header because of testing (and it's ifdef APPLE because the tests are also ifdef APPLE). If that is the case could we add a comment explaining that?



================
Comment at: lldb/unittests/Host/HostInfoTest.cpp:53-101
 #ifdef __APPLE__
 
-struct HostInfoMacOSXTest : public HostInfoMacOSX {
-  static std::string ComputeClangDir(std::string lldb_shlib_path,
-                                     bool verify = false) {
-    FileSpec clang_dir;
-    FileSpec lldb_shlib_spec(lldb_shlib_path, false);
-    ComputeClangDirectory(lldb_shlib_spec, clang_dir, verify);
-    return clang_dir.GetPath();
-  }
-};
-
+static std::string ComputeClangDir(std::string lldb_shlib_path,
+                                   bool verify = false) {
+  FileSpec clang_dir;
+  FileSpec lldb_shlib_spec(lldb_shlib_path, false);
+  ComputeClangDirectory(lldb_shlib_spec, clang_dir, verify);
----------------
I guess this test should move to mirror the change in code being tested (`unittests/Expression/Clang` ?).


https://reviews.llvm.org/D47384