[PATCH] D25587: Introduce llvm FormatVariadic

Duncan P. N. Exon Smith via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 13 18:49:46 PDT 2016


> On 2016-Oct-13, at 17:29, Zachary Turner <zturner at google.com> wrote:
> 
> Good suggestion. I wrote all the tests *before* adding the user defined literals. Check the very last test. That's how most people will probably use it in practice.

Oh, cool!  Totally missed it.

> 
> I will rewrite all the tests in terms of UDLs

It's not all bad to test the low-level layer... up to you.

> On Thu, Oct 13, 2016 at 5:26 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> I don't think I'll have bandwidth for a full review, but a couple of high-level comments off-the-cuff:
> 
> - create_formatted_string and format_string_object_base don't really following the LLVM coding conventions.  Should be createFormattedString and FormatStringObjectBase.  There may be other examples?
> 
> - From what I could see in the unittests, this looks fairly verbose to use vs. the current llvm::format.  Maybe I missed it, but it would be nice for the *safe* one to be easier to type.  Maybe rename llvm::format to llvm::formatPrintf or formatLegacy or something, and make this llvm::format?  Or is there a nice short name for this that doesn't conflict with llvm::format?
> 
> - It might be nice to add 1 or 2 uses in the actual code to see it in practice.
> 
> Excited for this!
> 
> > On 2016-Oct-13, at 17:19, Zachary Turner <zturner at google.com> wrote:
> >
> > zturner created this revision.
> > zturner added reviewers: chandlerc, timshen, dblaikie, dexonsmith, silvas, beanz.
> > zturner added a subscriber: llvm-commits.
> > Herald added subscribers: modocache, mgorny.
> >
> > As discussed on the mailing list, here is my initial pass at the variadic formatting library.
> >
> > Formatters are provided for integral, character, string, and pointer types.  More complex formatters such as those required for `chrono` classes are not yet provided, but they are lower priority.
> >
> > Note that this patch is blocked on https://reviews.llvm.org/D25497, so while I welcome reviews to this any time, it can't go in until that one is approved.
> >
> > Adding a bunch of people that expressed interest in this from the list.
> >
> >
> > https://reviews.llvm.org/D25587
> >
> > Files:
> >  include/llvm/ADT/STLExtras.h
> >  include/llvm/Support/FormatProviders.h
> >  include/llvm/Support/FormatVariadic.h
> >  include/llvm/Support/FormatVariadicDetails.h
> >  include/llvm/Support/NativeFormatting.h
> >  include/llvm/Support/YAMLTraits.h
> >  include/llvm/Support/raw_ostream.h
> >  lib/Support/CMakeLists.txt
> >  lib/Support/FormatVariadic.cpp
> >  lib/Support/NativeFormatting.cpp
> >  lib/Support/raw_ostream.cpp
> >  unittests/Support/CMakeLists.txt
> >  unittests/Support/FormatVariadicTest.cpp
> >
> > <D25587.74602.patch>
> 



More information about the llvm-commits mailing list