See also @pcc's proposal: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123514.html

Colocate CHECK lines with declarations

Why do we hardcode compiler IDs/versions here instead of just checking whether the current compiler supports the flag?

@ahatanak's suggestion

Ping.

Emailed cfe-dev: http://lists.llvm.org/pipermail/cfe-dev/2018-May/057934.html

If you're not planning to move forward with the diff, it's better to abandon it rather than close it. Close usually implies it was committed.

Yeah, I think we all agree now that a portability warning isn't really tractable. Note that even for the warnings that motivated this diff, they should have only fired if size_t and NSInteger had separate types, so it wasn't a portability warning in that sense to begin with (as in, it would only warn if there was a mismatch for your current target, not if there was a potential mismatch for any target).

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.

In D42933#1091943, @jyknight wrote:

In D42933#1091817, @jfb wrote:

In D42933#1091809, @jyknight wrote:

I also think that special casing these two specifiers doesn't make sense. The problem is a general issue -- and one I've often found irritating. This exact same situation comes up all the time in non-Darwin contexts too.

I don't think that's true. In this very specific case the platform guarantees that (sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == sizeof(NSUInteger)) for all architectures this platform supports. This exact same situation does not come up all the time in other contexts because the int / long / long long distinction isn't backed by a portability guarantee. The warning is there to say "this code isn't portable!", but in the very specific case of NSInteger and NSUInteger it is and users rely on it so it cannot be broken. The warning is therefore spurious, users therefore rightly complain.

The printf format specifier warning is not primarily a cross-platform portability checker. And, although in a few limited cases it can act somewhat like one, in general it does not. Take my previous example -- you get no warning on a platform that has int64_t as a typedef for long -- if this feature is to be useful as a portability checker, it should require that you used the PRId64 macro. Or, given ssize_t x = 0; printf("%ld", x);, it doesn't tell you to use "%zd" instead if ssize_t is a typedef for long -- although to be portable you ought to.

No, the major usefulness of the printf warning is to tell you that your code is incorrect for the _current_ platform. And most importantly when you chose the wrong size for your argument.

Those types which have matched size and alignment are still different types, and so it's technically appropriate to warn about using the wrong specifier there, too. But it's also entirely reasonable to not want to be bothered by such useless trivia, so skipping the warning when there's only a technical and not actual mismatch seems entirely sensible. (I might suggest that it should even be the default behavior for the warning, and if you want the stricter checks you'd ask for them...but I bet I'll get more pushback on that...)

Generating the patch from libc++ is fine (and this patch looks like it has sane paths).

In D42933#1091234, @aaron.ballman wrote:

In D42933#1090268, @jfb wrote:

I was just looking at this, and I think @arphaman's patch is pretty much the right approach (with 2 suggested fixes below).

I don't think the code we're currently warning on is broken: a user code has NSInteger with %zd or NSUInteger with %zu, and on all platforms which support those types the implementor has guaranteed that (sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == sizeof(NSUInteger)).

Yes, but is this guaranteed to be the case or does it happen to be the case? I'm worried about the less mainstream uses where you might find ObjC code where this does not hold but -Wformat points out a portability concern.

Note that the alignment matters in addition to the size.

Done testing

Is this fixing the same issue as D46214?

No worries; thanks for all the other reviews, and for adding Michael as a reviewer to this one :)

Makes sense to me. You may wanna wait for @zturner, but I can't really imagine any benefit to restricting this to MSVC.

Update with @grimar's suggestion

In D46099#1079143, @grimar wrote:

The new message is IMO slightly better, so I am OK with that patch.

But I am actually thinking about something like:
"Layout sections to place symbols in the order specified by symbol ordering file"

In D46099#1078997, @pcc wrote:

One could also read "Lay out symbols" as meaning the order of symbols in the symbol table, so I'm not sure if this is better. No strong opinions though.

(Gentle ping. This isn't urgent, but it would be nice to have.)

Only read CMAKE_EXE_LINKER_FLAGS, to be consistent with CMake under CMP0056

In D45639#1078494, @thakis wrote:

because the headers that are part of Clang toolchain are incompatible with the system library

Do you have details on this? This isn't supposed to be the case as far as I know. We link chrome/mac against system libc++ with the bundled headers, and it at least seems to work fine (and, from what I understand, is supposed to work as well).

LGTM. I'd tried to clean all stale _MSC_VER conditionals in the past, but I must have missed some.

Typo in the summary: refernece -> reference

LGTM; it's an obvious NFC patch.

Eric accepted on IRC.

This LGTM given that Eric accepted your previous change. I'm happy to accept it in a day or two if he hasn't gotten the chance to take a look by then.

In D45467#1062522, @zturner wrote:

I just noticed you said on non-Windows. Yea, I think the path should be whatever is native to the host you're building on.

Patch is missing context.

@ruiu just in case you didn't know, the URL https://llvm.org/PR36716 will do a redirect to the right place.

This doesn't pass all tests right now, and I'll also need to change the test in accordance with the review comments.

Finally got a chance to dig into some of the failures this patch is causing. Here's an example crasher (run with clang -cc1 -triple i686-windows -emit-llvm):

PURE_WINDOWS is all Windows except Cygwin. It's an LLVM thing, not a CMake thing.

Thanks Reid. I still need to look into why this is causing some existing tests to crash, but I'll also adjust the test.

Seems fine to me, since windows-itanium is unaffected.

I would imagine there was a reason this configuration was unsupported on macOS – perhaps something to do with libc++/libc++abi being the default system libraries on that platform?

Sweet, thanks for the update.

@DHowett-MSFT the reviewers look fine to me. Reid is the code owner for clang's MSVC compat support. David doesn't work on this stuff directly anymore, I think, but he's still pretty active in code reviews for it. I'm adding Saleem, who's also pretty active on Windows stuff.

LGTM with the comments addressed.

Could you upload the patch with context (-U9999) please?