[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

Sat Dec 10 10:09:29 PST 2016

To elaborate about member function vs format provider, decoupling types
from the way they're formatted seems like a good design practice in
general.  For example, you might want to add support for formatting a class
which you don't have the code to.  Sure, you can do it one way whenever
possible and another way whenever it's not possible, but since tight
coupling leads to more fragile code as a general rule, it seems better to
prefer the loose coupling whenever possible.

On the other hand, there are certain times when you *must* use a
member-based formatter.  For example, when something already has a
formatter defined for it (either via member syntax or format_provider
specialization) and you want to change the behavior.  A good example of
this is the `fmt_repeat` adaptor I use in this patch.  int already has a
formatter defined for it, so if we want to repeat an int 10 times, we have
no way to do that.  So we define a class `fmt_repeat` and give that a
format member function, then instead of saying formatv("{0}", 7) we say
formatv("{0}", fmt_repeat(7, 10));

Another point regarding F and D styles vs F.GetFilename() and
F.GetDirectory().  One of the original design goals was brevity, because
usually brevity helps readability.  To that end, I find

formatv("Dir: {0:D}, File: {0:F}", F);

more readable than

formatv("Dir: {0}, File: {1}, F.GetDirectory(), F.GetFilename());

There's another, more subtle, advantage though.  If you just use
GetFilename() and GetDirectory(), then what do you print if the string is
empty?  Here we've got a simple string, no different from any other string,
and an empty string is perfectly normal.  With specific types such as
FileSpec, you know more about where that string came from, so you can make
better decisions about how you want to handle the empty case (this argument
applies to anything, not just strings, btw).  So now you either have to
modify the core string formatter so that you can do something like this to
specify an "empty" value:

formatv("Dir: {0:(empty)}, File: {1:(empty)}", File.GetDirectory(), File.
GetFilename());

or, even worse, you have to do this:

formatv("Dir: {0}, File: {1}, File.GetFilename().IsEmpty() ? "(empty)" :
File.GetFilename(), F.GetDirectory().IsEmpty() ? "(empty)" :
F.GetDirectory());

both are getting further and further away from the very simple and easy to
read

formatv("Dir: {0:D}, File: {0:F}", F);

and both increase the possibility for errors and inconsistencies throughout
the code whereas with the F and D styles, you guarantee that every time you
ever print a FileSpec, it's always going to be consistent (of course, we
could add an override mechanism to the FileSpec formatter as well if it
were *really* necessary, but it seems best to try to avoid it as much as
possible)

On Sat, Dec 10, 2016 at 4:51 AM Zachary Turner <zturner at google.com> wrote:

> I would say format provider should be the default way to add formatting
> for a commonly formatted type. Member function syntax is used to implement
> adaptors that allow you to override formatting behavior of a type that
> already has a provider.
>
> One nice thing about F and D would be this:
>
> formatv("Dir: {0:D}, File: {0:F}", F):
>
> Here you only pass the argument once, but format it differently each time.
> You do have a point about not getting a compiler error. In the future i had
> planned to add compile time format string checking but it's a ways out. For
> now we assert and then fallback to the default case of an empty style.
>
> Also, i still haven't finished the Args stuff, so no risk of me going on a
> reformatting spree :)
> On Sat, Dec 10, 2016 at 1:03 AM Pavel Labath via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> labath added a comment.
>
> This looks like a step in the right direction. I trust you won't go on a
> reformatting spree of the log messages until we settle on the syntax there.
> ;)
>
>
>
> ================
> Comment at: include/lldb/Core/ModuleSpec.h:244
> +      strm.Format("object_mod_time = {0:x+}",
>                    uint64_t(llvm::sys::toTimeT(m_object_mod_time)));
>      }
> ----------------
> you can delete the `uint64_t` cast now. It was only there to make this
> portably printable.
>
>
> ================
> Comment at: include/lldb/Host/FileSpec.h:784
> +///
> +template <> struct format_provider<lldb_private::FileSpec> {
> +  static void format(const lldb_private::FileSpec &F, llvm::raw_ostream
> &Stream,
> ----------------
> I am wondering.. what is the preferred style for adding formatting
> capability. I sort of assumed that the `format_provider` thingy was
> reserved for cases where you had no control over the object being formatted
> (e.g. because it comes from a third party library), and that for cases like
> this we would use the member format function (or, to put it differently: if
> we're going to be using the `format_provider` everywhere, what's the point
> of having the member functions in the first place?).
>
> Also, my preference would be to not have the F and D styles for FileSpec.
> I think they are redundant, as we have FileSpec::GetFilename and
> FileSpec::GetDirectory, and also they are not compiler-enforced --
> `Formatv('{0}', fs.GetFilename())` is slightly longer than
> `Formatv('{0:F}', fs)`, but if I make a typo in the former one, it will be
> a compiler error, while the other one won't. I suppose it could be useful,
> if one wants to format say an array of FileSpecs using the array format
> syntax, but I'm not sure how often will someone want to do that *and*
> display only the filenames.
>
> I don't care strongly about any of these things, but I want to know what
> to expect.
>
>
> https://reviews.llvm.org/D27632
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20161210/b31c462f/attachment-0001.html>