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

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

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

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