[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

Tue May 29 14:49:43 PDT 2018

jfb added a comment.

In https://reviews.llvm.org/D47290#1113422, @aaron.ballman wrote:

> In https://reviews.llvm.org/D47290#1113412, @jfb wrote:
>
> > In https://reviews.llvm.org/D47290#1113365, @aaron.ballman wrote:
> >
> > > This is relaxing `-Wformat` and making users instead write `-Wformat-pedantic` to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?
> >
> >
> > From Shoaib's email:
> >
> > > Based on all the discussion so far, I think the consensus is that we don't want to relax -Wformat by default, but an additional relaxed option would be fine. That works for me (where I can just switch my codebases to use the new warning option), but it wouldn't handle JF Bastien's use case, so he would still want to pursue the relaxations specific to Apple platforms.
> >
> > This patch is specific to the Darwin platform, as proposed in Alex' original patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would be different and do what I think you're referring to.
>
>
> There were several people on the thread asking to not relax -Wformat for any target, but instead wanted a separate warning flag to perform a more relaxed check.
>
> http://lists.llvm.org/pipermail/cfe-dev/2018-May/057938.html

I don't think this one is talking about platform-specific warnings, I therefore don't think it applies to this patch.

> http://lists.llvm.org/pipermail/cfe-dev/2018-May/058030.html
>  http://lists.llvm.org/pipermail/cfe-dev/2018-May/058039.html

Hans' comment and your +1 are on-topic here, and are the only ones AFAIK (others don't care or explicitly agree with this patch). What I'd like to emphasize is that the new stricter warnings are causing issues for developers on our platform. The warnings are new, and they don't help developers because the platform guarantees that their code is fine. That frustrates developers because they *know* that we know better (they might even find this discussion when searching! 👋), yet we chose to come in and warn them anyways. I have yet to hear a reason why we're doing them a service by warning in this case. It really has the feel of a pedantic warning, and that's what this patch makes it.

I'll quote James <http://lists.llvm.org/pipermail/cfe-dev/2018-May/057996.html> slightly out of context:

> No, this actually has been a thorn in the side of some people at google,
>  causing a good deal of angst (only for a very small number of people,
>  granted). I'd not truly put the blame on the warning, but rather on printf
>  itself -- and that we're still USING printf (and other functions that
>  ultimately wrap printf) all over the place. The long-term plan is certainly
>  to switch to better APIs. But, it'd be nice to be able to reduce the issue
>  in the meantime.
> 
> The problem we have is that Google has an internal "int64" typedef, which
>  predates the availability of C99 "int64_t". We'd like to eliminate this,
>  and switch everything over to using the standard int64_t. (Well, really
>  we'd've liked to have done that years ago...) However, "int64" is defined
>  as "long long", while "int64_t" is defined as "long" on 64-bit linux. A
>  bunch of printf specifiers all throughout the codebase use %lld. This
>  mismatching type-specifier makes it quite difficult to change the
>  type. Automatically updating all the format strings has not been found to
>  be easy.
> 
> If we could eliminate the warning on mismatch of "%lld" vs "long", on a
>  platform where they're the same, that could make things a whole lot easier.

It seems Hans' comment came from the sentiment "warning is the right thing to do, and from Google's vast experience it's not a problem". James provides a counter-point in line with ours: we receive exactly that feedback for NSInteger and `%z`. The code exists, it works fine, the platform guarantees it'll keep working fine, and now a tonne of "useless" warnings come in. That's frustrating because it means developers have these options:

- Useless code churn in code that otherwise won't change, ever
- Silence the warning, potentially silencing useful warnings later (because -Wformat-relaxed could start doing more things)

That's frustrating when you update compilers, especially if your update is incremental because only some developers see the warning. It means people are reluctant to update, because the first thing they see is us being pedants, not the cool new features we bring in. If they saw a flood of really useful warnings instead they'd be happy,  but even there they get drowned by these format warnings... Kind of a shit sandwich if you'll pardon the graphic description.

Hopefully this makes sense? I'm not sure I summarize my context very well. I'm happy to talk about it in-person next week too.

Repository:
  rC Clang

https://reviews.llvm.org/D47290