Page MenuHomePhabricator

Add minnum / maxnum to APFloat
ClosedPublic

Authored by arsenm on Oct 9 2014, 7:14 PM.

Details

Reviewers
chandlerc

Diff Detail

Event Timeline

arsenm updated this revision to Diff 14695.Oct 9 2014, 7:14 PM
arsenm retitled this revision from to Add minnum / maxnum to APFloat.
arsenm updated this object.
arsenm edited the test plan for this revision. (Show Details)
arsenm added a subscriber: Unknown Object (MLST).
chandlerc added inline comments.
include/llvm/ADT/APFloat.h
339–341

I think these should be free functions. I know that breaks with historical patterns here, but we should start moving the API in that direction IMO (and others I've chatted about this API with seemed to agree).

As part of that, this needs to return by-value. As-is, this can return a dangling reference to a temporary argument.

arsenm updated this revision to Diff 14699.Oct 9 2014, 9:24 PM

Move to free function, return by value

Feel free to submit once suggestions are addressed.

include/llvm/ADT/APFloat.h
621–625

Please provide nice documenting comments for the semantics. Especially important to call out that these model IEEE's functionality and are not std::max.

Finally, why not inline? These seem reasonable inlining candidates....

chandlerc accepted this revision.Oct 9 2014, 9:31 PM
chandlerc added a reviewer: chandlerc.
This revision is now accepted and ready to land.Oct 9 2014, 9:31 PM
arsenm closed this revision.Oct 9 2014, 10:31 PM

Moved inline + comments in r219475