[FileSpec] Delegate common operations to llvm::sys::path
ClosedPublic

Authored by JDevlieghere on Tue, Jun 12, 10:16 AM.

Details

Summary

With the recent changes in FileSpec to use LLVM's path style, it is
possible to delegate a bunch of common path operations to LLVM's path
implementation. This means we only have to maintain a single
implementation and can benefit from the efforts of the rest of the LLVM
community.

This is part one of a set of patches. There was no obvious way to split
this so I just worked from top to bottom.

Diff Detail

Repository
rL LLVM
JDevlieghere created this revision.Tue, Jun 12, 10:16 AM
zturner accepted this revision.Tue, Jun 12, 10:29 AM

Looks like good cleanup to me, thanks!

source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
2593–2596 ↗(On Diff #150977)

Can you change these strcmp operations to StringRef calls? Might as well do some cleanup if we're here anyway.

This revision is now accepted and ready to land.Tue, Jun 12, 10:29 AM
labath added inline comments.Tue, Jun 12, 12:18 PM
source/Utility/FileSpec.cpp
244–246 ↗(On Diff #150977)

I don't really have a clear opinion on whether the new behaviour here is better or not, but it any case this sounds like something worth explicitly calling out.

unittests/Utility/FileSpecTest.cpp
284 ↗(On Diff #150977)

Any particular reason for removing this?

JDevlieghere marked an inline comment as done.Tue, Jun 12, 12:46 PM
JDevlieghere added inline comments.
source/Utility/FileSpec.cpp
244–246 ↗(On Diff #150977)

As far as the FileSpec class is concerned this doesn't change anything, we were always explicitly passing the m_style argument which is now implicit. To me this seems strictly better than what is was, were you might accidentally end up with the host path.

I'll add a comment in the header.

unittests/Utility/FileSpecTest.cpp
284 ↗(On Diff #150977)

Yup, llvm doesn't consider C: or //net to be absolute paths (as opposed to C:\ or //net/. While this is debatable, it makes things consistent with drive names on Windows, which is why I added the net tests above.

labath accepted this revision.Tue, Jun 12, 1:05 PM
labath added inline comments.
source/Utility/FileSpec.cpp
244–246 ↗(On Diff #150977)

Yes, but the counterargument to that is: since you're replacing the old filespec with a completely unrelated path, who's to say that the old path syntax is any better fit than "host".

I think it would be best here to actually make this argument mandatory and force everyone to think about the correct syntax when constructing the object, but maybe that's a change for another day.

unittests/Utility/FileSpecTest.cpp
284 ↗(On Diff #150977)

Ah, I see.. I can certainly understand why c: wouldn't be considered absolute, but I'm not sure the same thing applies to //net. However, I think I can live that interpretation right now..

JDevlieghere marked an inline comment as done.Tue, Jun 12, 2:16 PM
JDevlieghere added inline comments.
source/Utility/FileSpec.cpp
244–246 ↗(On Diff #150977)

Alright, I see your point, I was only thinking about its uses within the FileSpec class. I think we can solve both issues by making it mandatory publicly, and have a private function that maintains the current style.

davide accepted this revision.Tue, Jun 12, 2:52 PM

LGTM with a nit.

source/Utility/FileSpec.cpp
18–21 ↗(On Diff #150977)

This comments are really too trivial to be useful. Can you remove them?

labath added inline comments.Wed, Jun 13, 12:37 AM
source/Utility/FileSpec.cpp
244–246 ↗(On Diff #150977)

Agreed, maintaining the style for internal uses of SetFile makes perfect sense.

clayborg added inline comments.Wed, Jun 13, 7:34 AM
source/Utility/FileSpec.cpp
593 ↗(On Diff #150977)

We won't need the full path in order to get the extension. Should we just get the m_filename as a string here?

599 ↗(On Diff #150977)

We won't need the full path in order to get the extension. Should we just get the m_filename as a string here?

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked 11 inline comments as done.
JDevlieghere added inline comments.Wed, Jun 13, 9:27 AM
source/Utility/FileSpec.cpp
244–246 ↗(On Diff #150977)

This is pretty intrusive so I'll do this in a follow-up commit.