[libcxx-commits] [PATCH] D103433: [libc++][format] Adds integer formatter.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut accepted this revision.
vitaut added a comment.

Looks good, thanks for addressing the comments.



================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:392-393
+  // *** align-fill & width ***
+  // TODO FMT Validate whether this should be left aligned
+  test(STR("answer is '     *'"), STR("answer is '{:6c}'"), I(42));
+  test(STR("answer is '     *'"), STR("answer is '{:>6c}'"), I(42));
----------------
Mordante wrote:
> vitaut wrote:
> > Per my earlier comment this should indeed be left-aligned.
> I assumed that was intended. However my interpretation of the wording requires it to be right aligned:
> `This is the default for arithmetic types other than charT and bool or when an integer presentation type is specified.`
> My interpretation is that the supplied type in an aritmetic type, so this rule applies.
> Do you agree with my interpretation?
> 
> I agree it's silly that the result of `format("{:6c}", 42);` differs from the result of `format("{:6}", '*');`. So I'll adjust it.
> 
> 
Yeah, I think P1652 missed this part of the wording. The interpretation depends on whether you look at the original types or types after conversion per presentation specifiers. I suggest opening an LWG issue to clarify that we want the latter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103433/new/

https://reviews.llvm.org/D103433