Customize format for I32/U32 (issue #1920) #1927

krig · 2017-05-23T20:54:54Z

Fixes formatting of I32.min_value().

TODO: the u32 and u64 funs in _format_int.pony could probably be largely re-unified.

Fixes: #1920

SeanTAllen · 2017-05-24T01:25:16Z

There was a TravisCI failure that was unrelated to code, it was during dependency install. Restarted that one job.

SeanTAllen · 2017-05-24T01:25:57Z

packages/format/_format_int.pony

@@ -65,6 +65,40 @@ primitive _FormatInt
      s.reverse_in_place()
    end

+  fun u32(x: U32, neg: Bool, fmt: FormatInt, prefix: PrefixNumber,
+    prec: USize, width: USize, align: Align, fill: U32
+  ): String iso^ =>


see the style guide that is in progress for formatting on this.

#1894

Updated the PR. Something like this?

SeanTAllen · 2017-05-25T12:43:48Z

That would fit the forthcoming style guide @krig. Yes.

krig · 2017-05-25T14:09:38Z

Alright, great. Fixed one small formatting mistake in the previous update.

SeanTAllen · 2017-05-31T19:37:59Z

packages/format/format.pony

@@ -96,8 +96,10 @@ primitive Format
      _FormatInt.u128(x.u128(), false, fmt, prefix, prec, width, align, fill)
    elseif x is I128 then
      _FormatInt.u128(abs.u128(), neg, fmt, prefix, prec, width, align, fill)
-    else
+    elseif (x is U64) or (x is I64) then


there's a bug that predates your code @krig. These should be using a match expression or an iftype. The latter is preferred.

The is doesn't work as it compares identity rather than checking the type.

So x is I128 will only be triggered for the value 0. otherwise the else will always be used.

iftype A <: U128 then _FormatInt.u128(x.u128(), false, fmt, prefix, prec, width, align, fill) elseif A <: I128 then _FormatInt.u128(abs.u128(), neg, fmt, prefix, prec, width, align, fill) else _FormatInt.u64(abs.u64(), neg, fmt, prefix, prec, width, align, fill) end

Can you make that update @krig?

krig · 2017-05-31T20:52:41Z

@SeanTAllen I fixed the if/iftype bug, at least I think so. At the same time I thought to test if formatting min_value works for I8/I16 as well, since I suspected it didn't. And it didn't. So I implemented those as well. Ideas for how to reduce the duplication across the various int functions are welcome, I'm still very much a pony newbie :)

krig · 2017-05-31T20:58:38Z

Hm. It also doesn't work properly for ISize or ILong. Not sure how to solve those.

jemc · 2017-05-31T21:08:15Z

@krig Thanks for digging into this!

For implementing the variable-sized types (ISize, USize, ILong, ULong) properly, you'll want to use ifdef to detect ilp32 and/or llp64 in order to infer the bit width of the type on the current platform. To see what I mean, please take a look at the implementations of their respective bitwidth methods - you can copy the same ifdef condition they use in those methods to infer whether to treat them as 32-bit or 64-bit in the formatting code. For example, check out the definition of ILong.bitwidth.

krig · 2017-05-31T21:09:42Z

@jemc Ah, perfect! I'll update it.

jemc · 2017-05-31T23:05:28Z

packages/format/format.pony

+      _FormatInt.u16(abs.u16(), neg, fmt, prefix, prec, width, align, fill)
+    elseif A <: (U8 | I8) then
+      _FormatInt.u8(abs.u8(), neg, fmt, prefix, prec, width, align, fill)
+    elseif A <: (USize | ISize | ULong | ILong) then


Sorry, this isn't quite right - the *Size types have different bitwidth semantics from the *Long types. Please see the definition of ISize.bitwidth().

Ah, I see, thanks.

jemc · 2017-06-01T13:46:01Z

Sorry, looks like there's one more issue in the tests, where you're testing ISize but using the ifdef rule for ILong.

It's causing failures on the windows CI builds.

Fixes formatting of I32.min_value(), I16.min_value(), etc. TODO: the various type-specific funs in _format_int.pony could probably be largely re-unified. Update formatting to follow style guide proposal (ponylang#1894). Fixes: ponylang#1920

krig · 2017-06-01T13:54:06Z

Good thing I added the test.. alright, this should do it.

This is meant to prevent confusion with respect to other primitives whose value can be compared using `is ${PrimitiveName}`. Machine words are special primitives that can have multiple values, and comparing with `is ${MachineWord}` is currently equivalent to comparing to zero. Incidentally, fixing this also alerts us to the bug in #1920. We should hold off on merging this PR until after #1927 is merged.

jemc · 2017-06-02T12:36:59Z

Great, thanks for bringing this one home!

krig · 2017-06-02T13:22:41Z

Cool, thank you both!

SeanTAllen · 2017-06-02T17:05:11Z

Awesome to see this merged! Thanks @krig. You rock.

This is meant to prevent confusion with respect to other primitives whose value can be compared using `is ${PrimitiveName}`. Machine words are special primitives that can have multiple values, and comparing with `is ${MachineWord}` is currently equivalent to comparing to zero. Incidentally, fixing this also alerts us to the bug in #1920. We should hold off on merging this PR until after #1927 is merged.

SeanTAllen reviewed May 24, 2017

View reviewed changes

krig force-pushed the format-bug1920 branch from 5cf76eb to 48510fb Compare May 24, 2017 21:06

krig force-pushed the format-bug1920 branch from 48510fb to a03084a Compare May 25, 2017 14:07

SeanTAllen reviewed May 31, 2017

View reviewed changes

krig force-pushed the format-bug1920 branch from a03084a to e58fba3 Compare May 31, 2017 20:50

krig force-pushed the format-bug1920 branch from e58fba3 to de2581a Compare May 31, 2017 21:08

krig force-pushed the format-bug1920 branch from de2581a to 53dfce8 Compare May 31, 2017 21:14

jemc reviewed May 31, 2017

View reviewed changes

krig force-pushed the format-bug1920 branch from 53dfce8 to 4625c64 Compare June 1, 2017 12:21

krig force-pushed the format-bug1920 branch from 4625c64 to b12079b Compare June 1, 2017 13:53

jemc mentioned this pull request Jun 1, 2017

Change machine word constructors to have no default argument. #1938

Merged

jemc changed the title ~~Customize format for I32/U32 (#1920)~~ Customize format for I32/U32 (issue #1920) Jun 2, 2017

jemc merged commit d1a8f63 into ponylang:master Jun 2, 2017

jemc added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jun 2, 2017

jemc added a commit that referenced this pull request Jun 2, 2017

Update CHANGELOG for PR #1927 [skip ci]

d5e1966

krig deleted the format-bug1920 branch June 2, 2017 13:22

Praetonus mentioned this pull request Jul 7, 2017

Format.int[A] always formats as U64, even when A is U128 or I128 #2015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customize format for I32/U32 (issue #1920) #1927

Customize format for I32/U32 (issue #1920) #1927

krig commented May 23, 2017

SeanTAllen commented May 24, 2017

SeanTAllen May 24, 2017 •

edited

Loading

krig May 24, 2017

SeanTAllen commented May 25, 2017

krig commented May 25, 2017

SeanTAllen May 31, 2017 •

edited

Loading

krig commented May 31, 2017

krig commented May 31, 2017

jemc commented May 31, 2017

krig commented May 31, 2017

jemc May 31, 2017

krig Jun 1, 2017

jemc commented Jun 1, 2017

krig commented Jun 1, 2017

jemc commented Jun 2, 2017

krig commented Jun 2, 2017

SeanTAllen commented Jun 2, 2017

Customize format for I32/U32 (issue #1920) #1927

Customize format for I32/U32 (issue #1920) #1927

Conversation

krig commented May 23, 2017

SeanTAllen commented May 24, 2017

SeanTAllen May 24, 2017 • edited Loading

Choose a reason for hiding this comment

krig May 24, 2017

Choose a reason for hiding this comment

SeanTAllen commented May 25, 2017

krig commented May 25, 2017

SeanTAllen May 31, 2017 • edited Loading

Choose a reason for hiding this comment

krig commented May 31, 2017

krig commented May 31, 2017

jemc commented May 31, 2017

krig commented May 31, 2017

jemc May 31, 2017

Choose a reason for hiding this comment

krig Jun 1, 2017

Choose a reason for hiding this comment

jemc commented Jun 1, 2017

krig commented Jun 1, 2017

jemc commented Jun 2, 2017

krig commented Jun 2, 2017

SeanTAllen commented Jun 2, 2017

SeanTAllen May 24, 2017 •

edited

Loading

SeanTAllen May 31, 2017 •

edited

Loading