This is an archive of the discontinued LLVM Phabricator instance.

Remove dependency from Host to clang
ClosedPublic

Authored by zturner on May 25 2018, 11:35 AM.

Details

Summary

The idea is to move the discovery of the clang resource directory into a more appropriate place, such as the clang expression parser. By continuing with this pattern we can isolate all clang interactions to the clang expression parser. There is a little edge case here in that there is a public enumeration which is used in conjunction with SBHostOS to get a generic LLDB directory. So as not to break the public API, I just special case this particular value of the enumeration and don't pass it through to Host, rather passing it through to the clang expression parser plugin.

I'm not able to test this on OSX, so I expect there may be some easy to fix up compilation errors. Davide, would you mind helping me out with that?

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.May 25 2018, 11:35 AM

+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).

lldb/source/Plugins/ExpressionParser/Clang/ClangHost.h
17–20 ↗(On Diff #148640)

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?

lldb/unittests/Host/HostInfoTest.cpp
53–101 ↗(On Diff #148640)

I guess this test should move to mirror the change in code being tested (unittests/Expression/Clang ?).

Generally I'm fine with improving the layering. I just wanted to point out that the Swift language plugin also wants know the clang resource directory (it calls HostInfo::GetLLDBPath(ePathTypeClangDir, clang_dir_spec)) since Swift embeds Clang. That said it makes no sense to build an LLDB that supports Swift and not also support lang, so moving things into the Clang plugin should be fine.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2018, 10:45 AM
This revision was automatically updated to reflect the committed changes.
mclow.lists added inline comments.
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
77

This line fails to build on my old Mac Pro (Mac OS X 10.11, apple-clang-8), complaining that std::next requires a forward iterator, and rev_it is not one.

changing this code to:

auto parent = rev_it;
std::advance(parent, 1);

fixes the compile error.

Note that these are the most recent OS and tools for this computer.

Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 10:09 AM