This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Make functions that produce APFloaat objects use correct semantics.
ClosedPublic

Authored by timshen on Nov 2 2016, 6:33 PM.

Details

Summary

Fixes PR30869.

In D25977 I meant to change all functions that care about lifetime. I changed constructors, factory functions, but I missed member/free functions that return new instances. This patch changes them.

Event Timeline

timshen updated this revision to Diff 76810.Nov 2 2016, 6:33 PM
timshen retitled this revision from to [APFloat] Make functions that produce APFloaat objects use correct semantics..
timshen updated this object.
timshen added reviewers: hfinkel, kbarton, echristo, joerg.
timshen added a subscriber: llvm-commits.
hfinkel edited edge metadata.Nov 3 2016, 12:23 PM

Can you please explain the problem/solution?

llvm/include/llvm/ADT/APFloat.h
645

Why is S unused? If we're assuming that semantics is initialized elsewhere, a comment would be useful explaining why we have the argument.

timshen updated this revision to Diff 76885.Nov 3 2016, 2:09 PM
timshen edited edge metadata.

Actually use the Semantics that is passed into APFloat::Storage::Storage.

timshen updated this object.Nov 3 2016, 2:16 PM
timshen marked an inline comment as done.Nov 3 2016, 2:18 PM

Can you please explain the problem/solution?

Added. Sorry for the oversight!

Can you please explain the problem/solution?

Added. Sorry for the oversight!

llvm/include/llvm/ADT/APFloat.h
645

It should be propagated into DoubleAPFloat, but currently DoubleAPFloat doesn't need to support conversion between different semantics (it only has one!). Put an assert there, and someday DoubleAPFloat may change to actually consume this fltSemantic.

hfinkel accepted this revision.Nov 5 2016, 4:32 AM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 5 2016, 4:32 AM
This revision was automatically updated to reflect the committed changes.