Page MenuHomePhabricator

[NFC] Refactor InlineResult for readability
ClosedPublic

Authored by mtrofin on Jan 14 2020, 4:21 PM.

Details

Summary

InlineResult is used both in APIs assessing whether a call site is
inlinable (e.g. llvm::isInlineViable) as well as in the function
inlining utility (llvm::InlineFunction). It means slightly different
things (can/should inlining happen, vs did it happen), and the
implicit casting may introduce ambiguity (casting from 'false' in
InlineFunction will default a message about hight costs,
which is incorrect here).

The change renames the type to a more generic name, and disables
implicit constructors.

Diff Detail

Event Timeline

mtrofin created this revision.Jan 14 2020, 4:21 PM

Class Name ResultWithMessage sounds too generic. Why not keeping the InlineResult class name? The rest of the changes look reasonable.

Class Name ResultWithMessage sounds too generic. Why not keeping the InlineResult class name? The rest of the changes look reasonable.

Oh, because I read "InlineResult" as result of performing inlining, not also as a result of an inlinability analysis, but maybe I'm overthinking it.

InlineResult --> inlining related result (viability, etc) -- it captures two pieces of information: 1) inline decision and 2) when decision is 'no', related inline analysis that leads to the no decision. The class name seems fine. The patch makes the 'decision' part more explicit, and also fixes some bug in missing the right analysis message.

mtrofin updated this revision to Diff 238317.Jan 15 2020, 10:39 AM

Renamed back to InlineResult

davidxl accepted this revision.Jan 15 2020, 10:57 AM

thanks for the cleanup. The implicit conversion was not quite readable. LGTM

This revision is now accepted and ready to land.Jan 15 2020, 10:57 AM
This revision was automatically updated to reflect the committed changes.