- User Since
- Jan 29 2021, 10:57 AM (112 w, 2 d)
Dec 6 2022
Dec 2 2022
Fix new fixit test for Windows, where directory separators are backslashes instead of forward slashes.
Dec 1 2022
Addressed Aaron's comment about fixits, ran clang-format.
Nov 11 2022
Nov 10 2022
Nov 7 2022
Nov 2 2022
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.
Nov 1 2022
Fix whitespace difference that clang-format didn't like.
Oct 31 2022
Fix failing test when parsing "%" as a format string.
Oct 26 2022
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.
Oct 25 2022
Addressing final comments, running diff again for automated testing.
Oct 24 2022
Sep 16 2022
Sep 9 2022
Address @ahatanak's comments.
Sep 8 2022
Clang is being more zealous than the standard by warning you about this because integer-type variadic arguments undergo integer promotion (22.214.171.124: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 126.96.36.199:7, which describes format strings:
Aug 22 2022
Jul 5 2022
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).
Jul 1 2022
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).
Jun 22 2022
Would it be better if I asked a colleague to finish the review?
Jun 16 2022
Jun 8 2022
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.
May 12 2022
May 9 2022
Jan 5 2022
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 :)
Nov 12 2021
Nov 10 2021
Oct 29 2021
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.
Oct 28 2021
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.
Oct 27 2021
Oct 26 2021
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.
Feb 5 2021
Feb 3 2021
Feb 2 2021
Address Aaron's feedback
Jan 29 2021
Updating the diff using arcanist, which I'm told produces better results. Sorry for the churn!