(Responding to Hans first, then Hubert below)
The other thing to point out is, I’m dealing with codebases with thousands
of such instances.
This is why we care about this as well.
I haven’t heard of any issues coming from this internally at Google or
in Chromium, so while I understand you have more Cocoa code, it can’t
be that everything is broken by this.
I don’t understand where you want to go with this. Your and Google’s experience are relevant, but maintainers of the Darwin platform should be expected to hear much more about that platform, from a variety of codebase “flavors”. Google’s codebase, while very large, tends to be fairly uniform. Lack of data on your part can’t be extrapolated.
In the same way, Shoaib is representing Facebook’s experience. I don’t have the same experience, I’m nonetheless sympathetic to the point being made because it comes from a different codebase. I’m happy to let Shoaib speak up for that experience, and propose solutions which make sense for them.
We’ve cleaned up a lot of them (and the clang fix-its do come in handy
here), but at some point you just hit a wall and decide to go with
-Wno-error=format to be able to move forward. I’m trying to avoid throwing
the baby out with the bathwater here by adding an option to still retain
some of the format errors, at least. My plan is to actually go back and
clean up the rest of the issues as well, but this would allow us to do it in
a staged fashion instead of an all-or-nothing.
It sounds like a -Wformat-relaxed variant would serve this approach.
This indeed helps Facebook’s codebase, and I’m happy for this outcome. However, it doesn’t resolve the original problem raised in D42933. If folks want to go with two solutions it’s OK. The original email was trying to solve both issues at once.
I’m still non-plussed by the proposed name, though. “Relaxed” is awfully vague, when what’s being relaxed are specific portability warnings.
I’d also like to point out John’s response from D42933:
! In D42933#1092048, @rjmccall wrote:
I agree that the format-specifier checker is not intended to be a portability checker.
Any attempt to check portability problems has to account for two things:
- Not all code is intended to be portable. If you’re going to warn about portability problems, you need some way to not annoy programmers who might have good reason to say that they only care about their code running on Linux / macOS / 64-bit / 32-bit / whatever. Generally this means splitting the portability warning out as a separate warning group.
- There are always established idioms for solving portability problems. In this case, a certain set of important typedefs from the C standard have been blessed with dedicated format modifiers like “z”, but every other typedef in the world has to find some other solution, either by asserting that some existing format is good enough (as NSInteger does) or by introducing a macro that expands to an appropriate format (as some things in POSIX do). A proper format-portability warning would have to know about all of these conventions (in order to check that e.g. part of the format string came from the right macro), which just seems wildly out-of-scope for a compiler warning.
I’m not sure -Wformat-relaxed is a good solution, but again if that’s what y’all want let’s do it, as well as something to resolve the original problem raised in D42933.
It sounds like the problem is really that NSInteger and size_t are
typedeffed to different types on 32-bit. Would it be possible to get
that fixed instead?
NSInteger is, on purpose, the same size as a size_t. The types can’t change.
(It’s a shame they weren’t typedefed to the same type then; but I
suppose I should let that go.)
Yes please 
Is this documented somewhere? As pointed out on the review, Apple
recommends casting and printing NSInteger with %ld. Where does the
practice of using %zd come from?
Quoting John again:
I’m not saying that the recommendation would change, but long
being pointer-sized is a pretty universal assumption on Darwin, I’m afraid.
In the review I also said I’d update the documentation once this issue is resolved:
https://reviews.llvm.org/D42933#1091502
Anyway, this proposal is much broader than handling the NSInteger vs
%zd issue, as it would suppress all kinds of argument mismatches as
long as the size and alignment matches on the target. I think that
would be very unfortunate, but I can see a place for an off-by-default
relaxed variant of the warning for code that doesn’t care about it.
Indeed, this proposal is broader. D42933 suggest a very specific fix for NSInteger and %z formats on Darwin platforms. Shoaib suggested we broaden the fix based on Facebook’s experience with portability warnings. I’m happy to broaden the fix if it addresses the original problem. If we pursue something which doesn’t address the original problem then I want to pursue a separate fix for that problem, and we’re back to the original suggestion in D42933.
It’s (mostly) not trying to be clever; it’s just checking what the
standard requires. Printing an int with %ld is wrong according to the
standard even if int and long are the same size on the target.
Many things are wrong according to the standard and we don’t warn about it
because the standard is being silly. In this case I don’t see a technical
reason why integral types with matching sizeof and signed-ness should be
incompatible. There is a portability problem if you recompile and the
platform doesn’t make sure the format specifier and integral types aren’t in
sync, but as explained in the patch that’s guaranteed to work on Darwin
platforms.
So the argument is that code compiled on Darwin doesn’t care about
portability? I don’t think the standard is being particularly silly
here and I don’t expect it to change.
No. The argument is that NSInteger with %z is guaranteed to always work on Darwin, therefore there is no portability problem on Darwin and the warning is a false-positive. False-positive warnings shouldn’t be on at -Wall, especially at the quantity of warnings it emits in user code (this one is particularly noisy).
There’s a separate argument on the futility of portability warnings, for that see the quote from John above.
I prefer to think of them as correctness warnings rather than
portability warnings. I’m not buying the argument that because it
doesn’t catch all portability issues, it shouldn’t warn about
incorrect code.
In the very specific case of NSInteger and %z on Darwin platforms, do we
agree there’s no correctness issue? I’m not talking standards correctness,
I’m talking “the code does what’s expected”.
Sure.
But the current proposal seems much broader than that. And it seems
like it would be a shame to hack around just this specific issue in
the compiler.
I hope my response above answered this point.
I would prefer not to relax -Wformat by default. I think what it’s
doing is simple and correct, and a large amount of code compiles with
it.
We’d be relaxing something that only just got added to clang, and which
tries to catch portability issues.
-Wformat has been in a long time. From what I understand it’s just
that it recently started caring about signed %z variants.
I think we agree here.
It’s evidently not doing this very well
because it doesn’t know about platform guarantees. It’s a useful warning in
general, but the false positive rate is high, at least on Darwin platforms.
That’s why I want to “relax” -Wformat: because it’s noisy and people will
ignore it.
It’s been doing well so far. It sounds like the false positive rate is
due to a specific problematic problem on a specific platform, so it
seems a shame to downgrade the warning overall just because of that.
No, there are 2 distinct problems: one on Darwin, one on Facebook’s codebase. Shoaib suggested addressing both issues with one fix. We might decide to address both separately. Shoaib’s proposal is indeed further-reaching, hence the cfe-dev proposal.
I’m not opposed to adding a -Wformat-relaxed option if there’s a lot
of demand for it.
I’m opposed to having these format portability warnings on with -Wall. -Wall
shouldn’t be erroneously noisy.
As above, it seems the noise is contained to a single problem on a
single platform. I think the warning overall is high quality.
2 problems on at least 2 platforms. We haven’t heard from other platforms, which doesn’t mean it’s restricted to 2 platforms. A warning working well in your experience is a good datapoint, but by no means is it the only valid datapoint.
Cheers,
Hans
Great! Sorry there’s a lot of context to ingest.
Sure. The reason I bring back D42933 is, as I’ve stated to Hans above, that if this proposal doesn’t address D42933 then we still need to address D42933. Further, D42933 is relevant because it’s a fairly similar datapoint to Facebook’s, albeit much more narrow.
Opt-in by the platform was the original D42933 proposal. It sounds like you’re OK with that approach if Shoaib’s proposal goes in another direction?
This is one of the reasons I dislike -Wformat-relaxed as a name. Relaxed what?
I don’t agree with you here, and defer to John’s comment (quoted above) for why.