This is an archive of the discontinued LLVM Phabricator instance.

[support] Support printing floats in ScopedPrinter
ClosedPublic

Authored by paulkirth on Mar 3 2023, 3:27 PM.

Details

Summary

llvm-readobj will need the ability to print floats for use in
HashHistograms. This adds that functionality to the ScopedPrinter and
JSONScopedPrinter.

Diff Detail

Event Timeline

paulkirth created this revision.Mar 3 2023, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 3:27 PM
paulkirth requested review of this revision.Mar 3 2023, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 3:27 PM
paulkirth updated this revision to Diff 502709.Mar 6 2023, 10:33 AM

Fix formatting

jhenderson added inline comments.Mar 7 2023, 12:57 AM
llvm/include/llvm/Support/ScopedPrinter.h
237

Would it make sense to add a double overload too, whilst you're at it? I think it might be a little confusing having one but not the other. (I can see the argument against it, since it's not going to be used immediately though, so happy either way)

llvm/unittests/Support/ScopedPrinterTest.cpp
560
paulkirth added inline comments.Mar 7 2023, 10:07 AM
llvm/include/llvm/Support/ScopedPrinter.h
237

I think that is a good idea, and is probably more consistent w/ JSON representation anyway.

llvm/unittests/Support/ScopedPrinterTest.cpp
560

Good catch. Thank you.

paulkirth updated this revision to Diff 506265.Mar 17 2023, 8:12 PM

Address comments.

  • add overloads for double
  • add test w/ infinity
jhenderson accepted this revision.Mar 20 2023, 2:42 AM

Looks good, but see inline comment for possible suggestion.

llvm/unittests/Support/ScopedPrinterTest.cpp
605

Just a thought, but this value is implementation defined ultimately. I'm not suggesting necessarily changing this, but if you don't, you should keep a close eye on CI after landing this in case it doesn't work for a particularly system.

The alternative would be to build this string up dynamically, using std::to_string on the same values as the ones passed to printNumber.

This revision is now accepted and ready to land.Mar 20 2023, 2:42 AM
paulkirth updated this revision to Diff 506705.Mar 20 2023, 1:51 PM

Update test to avoid implementation specific output.

This revision was automatically updated to reflect the committed changes.

Hi, I think this patch is causing a failure on the clang-ppc64-aix bot here https://lab.llvm.org/buildbot/#/builders/214/builds/6531/steps/6/logs/FAIL__LLVM-Unit__83 . Would you be able to take a look?

Sorry for the trouble. looking now

so it looks like there are additional platform specific behaviors to how things like infinite are printed (inf vs INF, etc.). @abhina.sreeskantharajan I should be able to forward fix this fairly soon, probably in an hour(commute + meeting), but all that should be required is to adjust the test string the same way we do for max doubles/floats so that it's dynamically created and will thus match on all platforms.