[libcxx-commits] [PATCH] D103368: [libc++][format] Adds parser std-format-spec.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:132
+  // character. Validate what fmt and MSVC have implemented.
+  _CharT __fill{_CharT(' ')};
+
----------------
vitaut wrote:
> vitaut wrote:
> > Mordante wrote:
> > > vitaut wrote:
> > > > This should really be a grapheme cluster but {fmt} uses a code point at the moment (grapheme clusterization will be added later). In any case this should be a range (e.g. string_view) and not a single code unit.
> > > My interpretation of http://eel.is/c++draft/format#string.std-2
> > >   The fill character can be any character other than { or }.
> > > is that this is intended to be a single character and not a Unicode grapheme cluster. I can't find a paper or defect report that intends to change the current behaviour. Was it intended to be part of P1868R2 "🦄 width: clarifying units of width and precision in std::format" ?
> > > 
> > > Is there a paper or defect report, or should I file a defect report?
> > > 
> > > I don't mind changing it, but for now there are some unclarities how to implement this feature:
> > > Are 2 column Unicode characters allowed?
> > > - if yes, how to do odd even formatting?
> > > - if no, then is it a precondition or should the implementation do an width estimate?
> > > 
> > > For now I'll leave it as is, there's a `TODO FMT` to address this issue later.
> > > should I file a defect report?
> > 
> > Please do open a LWG issue. The "character" term is ambiguous but if interpreted in the C++ sense the current wording is clearly wrong because you cannot even use box drawing characters (in the Unicode sense). We can discuss the details in the LWG issue.
> So have you or do you plan to open an LWG issue? Otherwise I can do it.
I plan to file an LWG issue. But I first want to address all other review comments. It's on my TODO list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103368