Page MenuHomePhabricator

Refactor ObjectFile::GetSDKVersion
ClosedPublic

Authored by teemperor on Apr 27 2019, 12:56 PM.

Details

Summary

This patch modernizes the GetSDKVersion API and hopefully prevents problems such as the ones discovered in D61218.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Apr 27 2019, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2019, 12:56 PM
  • Added documentation on the return value.

I decided to go for a vector because otherwise the implementation in ObjectFileMachO gets a bit awkward. But I have no problem with doing a VersionTuple instead and adding some manual translation to version tuple in ObjectFileMachO.

labath added a subscriber: labath.Apr 29 2019, 1:20 AM

I decided to go for a vector because otherwise the implementation in ObjectFileMachO gets a bit awkward. But I have no problem with doing a VersionTuple instead and adding some manual translation to version tuple in ObjectFileMachO.

I think it would be better to use a VersionTuple, for consistency with other version uses. I don't see why the MachO implementation should be awkward because of that. You'd just need to replace the three push_back lines with m_sdk_version = VersionTuple(xxxx, yy, zz);. In fact, if at the same time you rewrite the GetSDKVersion into something like:

if (m_sdk_version.empty())
  m_sdk_version = GetVersionFromLC_VERSION());
if (m_sdk_version.empty())
  m_sdk_version = GetVersionFromLC_BUILD_VERSION());
return m_sdk_version;

that code will become at least five times more readable than it is now. :)

aprantl accepted this revision.Apr 29 2019, 12:32 PM

Awesome. I should have done that right away. This isn't super important, but I feel like just returning a vector (with potentially zero elements would be just as good) and the Optional doesn't add much value.

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
211 ↗(On Diff #196987)

For my own education, why is this preferred over versions[0]? Because of Optional's operator->?

This revision is now accepted and ready to land.Apr 29 2019, 12:32 PM
teemperor updated this revision to Diff 206451.Jun 25 2019, 7:59 AM
  • Rewrote patch to use llvm::VersionTuple (thanks Pavel)
teemperor marked an inline comment as done.Jun 25 2019, 7:59 AM
teemperor added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
211 ↗(On Diff #196987)

Yeah, just preferred that style to (*versions)[0] :)

labath added a comment.Jul 1 2019, 1:09 AM

looks good to me too...

clayborg accepted this revision.Jul 1 2019, 8:35 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 3:25 PM