Fix new fixit test for Windows, where directory separators are backslashes instead of forward slashes.

Addressed Aaron's comment about fixits, ran clang-format.

Merged to mainline; sadly forgot to update the commit message to include the differential ID, so manually closing. I'll see if I can find existing git hooks that check for that in the future.

Fix whitespace difference that clang-format didn't like.

In D136805#3887165, @efriedma wrote:

since the undefined behavior was evaluated at build time, UBSan wouldn't catch it.

Can you file a bug for this? Opportunistic constant evaluation like that shouldn't bypass ubsan.

Fix failing test when parsing "%" as a format string.

Add comment explaining what specifierBit does.

I will revert now and continue the investigation since it has been about 15 minutes.

That's what it complained about: https://buildkite.com/llvm-project/premerge-checks/builds/118482#01840c08-5dd5-40e8-8ebd-bcab1405d1ff

Failing tests are from clangd and WebAssembly backend, which are not touched by this change.

Addressing final comments, running diff again for automated testing.

Address @ahatanak's comments.

Clang is being more zealous than the standard by warning you about this because integer-type variadic arguments undergo integer promotion (6.5.2.2:6 in n2310 numbering). It is effectively impossible to pass an integer smaller than int to a variadic function. This means %hu and %hhu are constraints that the format function needs to read an unsigned short or unsigned char from the va_list, but anything smaller than unsigned will be converted to unsigned as it is being passed to the variadic function, and va_arg(ap, unsigned char) is necessarily equivalent to (unsigned char)va_arg(ap, unsigned). This is reiterated and explicitly spelled out in 7.21.6.1:7, which describes format strings:

There was a merge conflict on the release notes, updating the differential to get a CI build.

Address documentation comments.

I'm afraid that's also not possible: D is a Decl, so it doesn't have getType(). Decl is the tightest-fitting superclass of BlockDecl, FunctionDecl and ObjCMethodDecl (because BlockDecl is a direct subclass of it).

Thanks, Aaron. I wasn't sure how to follow up given how long it had been since the review started. I understand that we're all busy (which explains the week delay on my part here as well).

Would it be better if I asked a colleague to finish the review?

Ping

Apologies the long delay: things happened and I was pulled away. I have some time to finish this change now. I recommend re-reading the discussion up to now since it's not _that_ long and it provides a lot of very useful context.

I think that this is a good warning and I'll defer to the experts for what has to happen when prototypes merge with K&R definitions :)

Thanks Arthur for your feedback.

Thanks for looking, Aaron. You're right that the main utility of the aggregation of format warnings is to extend C's type checking because there is no other good way, or good place, to do it. I have built hundreds of millions of shipping lines of C, C++ and Objective-C, and this change seems like it would be an effective fix in several places where we don't currently have anywhere else to go.

Add test for a protocol method with format_arg, second NSString method accepting a NSString instead of a C string

Apologies, Phabricator showed the comment below line 197 in the diff, but the email showed it to be below line 3404. I can check if the return type is instancetype in handleFormatArgAttr and use that instead.

Forgot to run clang-format.

Thanks Artem for pointing out that I was completely misusing getFunctionScopeIndex. This should be better. I added a test that you can pick a non-1 value for the format parameter in blocks.

Address Aaron's feedback

Updating the diff using arcanist, which I'm told produces better results. Sorry for the churn!