Page MenuHomePhabricator

[ExpressionParser] Test GetClangResourceDir

Authored by xiaobai on Feb 27 2019, 4:13 PM.



I'm doing this because I plan on implementing ComputeClangDirectory on windows
so that GetClangResourceDir will work. Additionally, I made
test_paths make sure that the directory member of the returned FileSpec is not
none. This will fail on windows since ComputeClangDirectory isn't implemented

Diff Detail


Event Timeline

xiaobai created this revision.Feb 27 2019, 4:13 PM
teemperor added inline comments.

Doesn't that mean that we no longer test any of the paths on windows? E.g. if someone breaks lldb.ePathTypeLLDBShlibDir we will not see this test failing on Windows. I would prefer of extracting the ClangDir test into it's own method that we mark as failing.

xiaobai added inline comments.Feb 28 2019, 11:11 AM

Technically it is being tested but it is expected to fail. That being said, I don't mind extracting the ClangDir test into its own method while I work on implementing it, but I don't think it needs its own test. Maybe after I implement GetClangResourceDir on windows I can merge the tests? :P

labath added inline comments.Feb 28 2019, 11:46 AM

That sounds fine, though I have to say that these tests don't really test much. There is a test for the MacOS functionality here in unittests/Expression/ClangParserTest.cpp, which is much more detailed. It would be great if you could add a windows version there too.

xiaobai marked an inline comment as done.Feb 28 2019, 12:06 PM
xiaobai added inline comments.

I originally added this test there, but testing things there as-is is presents a disadvantage: The unittests don't link against liblldb but the individual lldb libraries, so the methods that figure out where the Clang Resource Dir (and pretty much any other directory for that matter) will report something like this:
/path/to/llvm/build/tools/lldb/unittests/lib/clang/9.0.0. So, the best this test could do is set that the filespec had a directory and not a filename set, which is what this test already does.

That being said, the way MacOS is being tested is much nicer. I could do a small refactor so the non-MacOS posix implementation is closer to the MacOS implementation and add a Linux test (and eventually a windows test) to unittests/Expression/ClangParserTest.cpp as well. What do you think?

labath added inline comments.Feb 28 2019, 12:29 PM

Sounds like a plan.

xiaobai updated this revision to Diff 188816.Feb 28 2019, 5:23 PM

Address suggestions and add unit test

xiaobai updated this revision to Diff 188817.Feb 28 2019, 5:26 PM

Reword comment to be clearer

xiaobai planned changes to this revision.Feb 28 2019, 6:13 PM

Looks like the test I wrote doesn't pass on MacOS, I need to investigate.

xiaobai updated this revision to Diff 188952.Mar 1 2019, 11:42 AM

Minor changes to make MacOS work

labath added a subscriber: aprantl.

Looks fine to me, but maybe @aprantl could give this a quick look, as he's the one (IIRC) who introduced this function.

@aprantl: Do you mind taking a look at this when you get a moment?

Looks mostly good!


While you are at it, could you rename this DefaultComputeClangResourceDirectory?
And add a doxygen comment that says it will compute the calng resource directory assuming that clang was installed to the same prefix as lldb?

xiaobai updated this revision to Diff 189392.Mar 5 2019, 1:11 PM

{Default,}ComputeClangDirectory -> {Default,}ComputeClangResourceDir
Added a comment explaining exactly how DefaultComputeClangResourceDir works

aprantl accepted this revision.Mar 5 2019, 3:07 PM
This revision is now accepted and ready to land.Mar 5 2019, 3:07 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 4:46 PM

This introduced a build break on Windows (which happened to coincide with another build break).