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

[Pavel Labath via lldb-commits]

On 10 December 2016 at 18:09, Zachary Turner <zturner at google.com> wrote:
> 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.

Thanks for explaining this. Loose coupling sounds very reasonable.
Reading the docs again, they do seem to imply that this is the
preferred method. I think what confused me is that I started off with
the source code, and that one searches for the member function first.
:)

>
> 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));

In this case, I have a feeling that the search for a member function
was implemented in a more complicated way than necessary (and I have
made it more complicated without understanding the reasoning behind
it). But that's a different discussion. I'll try to send you a patch
simplifying it.


> 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)
>

Seems reasonable. I withdraw my objection. LGTM.