This is an archive of the discontinued LLVM Phabricator instance.

[DWARFASTParserClang] Factor out structure-like type parsing, NFC
ClosedPublic

Authored by vsk on Oct 3 2019, 2:11 PM.

Details

Summary

Split out the logic to parse structure-like types into a separate
function, in an attempt to reduce the complexity of ParseTypeFromDWARF.

Note: Phab seems to be mis-rendering the diff. I'd be happy to move this
review to the mailing list to make things easier.

Inspired by discussion in https://reviews.llvm.org/D68130.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Oct 3 2019, 2:11 PM
shafik added a comment.Oct 3 2019, 3:45 PM

Thank you for doing this, this looks like an excellent start from a quick review.

vsk added a comment.Oct 3 2019, 3:50 PM

Thank you. I've just realized that Phab's "Raw Diff" mode (https://reviews.llvm.org/F10156020) renders the diff in a much clearer way. I suspect reviewing the raw diff will be much easier than reviewing what Phab has rendered here.

labath accepted this revision.Oct 3 2019, 11:58 PM

Looks good to me. Thanks for doing this. Personally, I'd just remove the Log argument from the function argument list, and let the function re-fetch it if needed. I know we sometimes pass around a Log* variable, but most of the time we don't...

This revision is now accepted and ready to land.Oct 3 2019, 11:58 PM
vsk added a comment.Oct 4 2019, 6:41 PM

Looks good to me. Thanks for doing this. Personally, I'd just remove the Log argument from the function argument list, and let the function re-fetch it if needed. I know we sometimes pass around a Log* variable, but most of the time we don't...

That's a good idea. I'll post an updated patch -- as it's relatively minor, I'll land it Monday afternoon (PST) or so if I don't see any objections.

vsk updated this revision to Diff 223357.Oct 4 2019, 6:51 PM
  • Remove the log parameter from ParseTypeFromDWARF.
teemperor accepted this revision.Oct 5 2019, 12:10 AM

Also LGTM! Thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 10:16 PM