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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
