Page MenuHomePhabricator

Remove remaining platform specific code from FileSpec
ClosedPublic

Authored by zturner on Mar 19 2017, 10:22 AM.

Details

Summary

This is the last step before I plan to move FileSpec to Utility, which should completely eliminate between 3 and 5 dependencies from other targets to Host.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Mar 19 2017, 10:22 AM
zturner updated this revision to Diff 92287.Mar 19 2017, 10:49 AM

Turns out ResolveUsername was only being called from one place outside of FileSpec and ResolvePartialUsername was dead code (since callers had already been updated to use TildeExpressionResolver. So I just deleted these two functions and replaced the calls to ResolveUsername with calls directly into TildeExpressionResolver.

labath added inline comments.Mar 20 2017, 4:02 AM
lldb/include/lldb/Utility/TildeExpressionResolver.h
62 ↗(On Diff #92287)

I find it very strange to see the Mock object in a regular header file (and I also don't see a reason why we would need that). Could we get rid of this? (perhaps by declaring it in a test-only header file if necessary)

zturner updated this revision to Diff 92373.Mar 20 2017, 1:06 PM

See what you think about this. I've created a folder called Mocks under Utility, and created a new target out of it. UtilityTests links against it, and so does InterpreterTests. To do this I had to add the lldb project root as an include directory, this way you can write #include "unittests/Utility/Mocks/MockTildeExpressionResolver.h". Another possibility would be to create lldb/unittests/Mocks/Mocks/Utility, and then add lldb/unittests/Mocks as an include folder. This way you could write #include "Mocks/Utility/MockTildeExpressionResolver.h". I don't have a strong preference either way.

I explicitly chose NOT to make one single target for all mocks (although there is only one right now obviously), because then you would have UtilityTests bringing in mocks from projects it doesn't depend on, and thus it would link against those projects as well, breaking the library layering.

labath edited edge metadata.Mar 21 2017, 5:22 AM

See what you think about this. I've created a folder called Mocks under Utility, and created a new target out of it. UtilityTests links against it, and so does InterpreterTests. To do this I had to add the lldb project root as an include directory, this way you can write #include "unittests/Utility/Mocks/MockTildeExpressionResolver.h". Another possibility would be to create lldb/unittests/Mocks/Mocks/Utility, and then add lldb/unittests/Mocks as an include folder. This way you could write #include "Mocks/Utility/MockTildeExpressionResolver.h". I don't have a strong preference either way.

I explicitly chose NOT to make one single target for all mocks (although there is only one right now obviously), because then you would have UtilityTests bringing in mocks from projects it doesn't depend on, and thus it would link against those projects as well, breaking the library layering.

It's not exactly the pinnacle of elegance, but I think this is better (and I don't know how I would improve it). The only thing I am not sure about is the Mocks folder name. I am thinking that we may run into the situation where we have some other test-only code we want to share, but it is not strictly a "mock". The only function that comes to mind right now is the CreateConnectedSockets from SocketTest.cpp -- I could have to reused this in gdb-remote tests, although I eventually opted for reimplementing it, as I could make it a lot simpler for this special case. (Although in this case it may have made sense to add a function like that to the regular codebase). I don't really have a good suggestion for a name though, so maybe we can leave this until a real case for it comes up.

eugene accepted this revision.Mar 21 2017, 11:31 AM

I like how much code you deleted.

This revision is now accepted and ready to land.Mar 21 2017, 11:31 AM
This revision was automatically updated to reflect the committed changes.