This is an archive of the discontinued LLVM Phabricator instance.

Fix PR #12906: printing of floating-point constants on non-C locales
Needs ReviewPublic

Authored by pitrou on Jan 27 2016, 9:38 AM.
This revision needs review, but all specified reviewers are disabled or inactive.

Details

Reviewers
tstellarAMD
Summary

Replace the libc-based printing with calling out to APFloat.toString().
Unfortunately the representation format is slightly different, which produces a lot of churn in the test suite.

Diff Detail

Event Timeline

pitrou updated this revision to Diff 46146.Jan 27 2016, 9:38 AM
pitrou retitled this revision from to Fix PR #12906: printing of floating-point constants on non-C locales.
pitrou updated this object.
pitrou added a subscriber: llvm-commits.

Note the test I added to AsmWriterTest.cpp may not be acceptable (as it changes the locale), but it showcases the issue for review.

(by the way, a related submission awaiting review is at http://reviews.llvm.org/D15375)

majnemer added inline comments.
lib/IR/AsmWriter.cpp
1113

Does other code in LLVM use this overload of left-shift? If not, I'd remove it. If so, I'd consider moving the change to the ostream code.

pitrou updated this revision to Diff 46240.Jan 28 2016, 1:02 AM
pitrou edited edge metadata.

Updated patch folds the change into raw_ostream::operator<<(double N).

pitrou added inline comments.Jan 28 2016, 1:03 AM
lib/IR/AsmWriter.cpp
1113

Yes, the overload is used elsewhere. I've uploaded a new patch that moves the change there, and reconciles float representation between APFloat and raw_ostream.

Hi,
Is there anything else you want me to do on this patch?
Thank you

Hi,
I know this patch is a bit hard to swallow given the number of trivial changes in the test suite, but it would be nice to get feedback, at least on the overall approach. As fas as I can tell, the bug is still present in git master.

I know this patch is a bit hard to swallow given the number of trivial changes in the test suite, but it would be nice to get feedback, at least on the overall approach. As fas as I can tell, the bug is still present in git master.

It looks fine to me, except as you say the large number of test changes make it scary. It seems to me that it'd be nice to preserve the existing output (via an argument passed to APFloat::toString), just for sanity's sake even though it's not required for compatibility or anything.

Dunno if others agree?