[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

Noah Goldstein via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 18 10:37:05 PST 2022


goldstein.w.n added a comment.

In D137181#3935951 <https://reviews.llvm.org/D137181#3935951>, @owenpan wrote:

> In D137181#3935856 <https://reviews.llvm.org/D137181#3935856>, @goldstein.w.n wrote:
>
>> In D137181#3935752 <https://reviews.llvm.org/D137181#3935752>, @owenpan wrote:
>>
>>> Please mark review comments as done if you have addressed them. Can you also clean up the test cases, removing overlapping/redundant ones, making sure a test case doesn't end with a newline (e.g., line 5380), etc?
>>
>> Regarding newlines. I have new line before changing the style. Otherwise done. (let me know if you want me to remove those).
>
> Sorry, it's line 5379 and several other places that have an ending newline. (Just search for `\n",`.) You only need to remove them in the new test cases. We can fix the existing ones in a separate NFC patch.

Done. Sorry I had thought you mean the newline in the file between the test cases.

>> Regarding removing redundant test cases. I think I've had each of them fail independently at some point (although many when I was doing the previous PPLevel tracking) so I wouldn't call any of them truly redundant.
>
> I know, but IMO some of the failures (e.g. the `switch` statement) during development shouldn't automatically go in. This file already has a humongous size. Maybe do the best you can here.
>
>> I could remove either the `PPDIS_BeforeHash` or `PPDIS_AfterHash` though as the they really no independent logic in this commit.
>
> Yes, please. Or better yet, alternate a little bit between the two.

Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137181/new/

https://reviews.llvm.org/D137181



More information about the cfe-commits mailing list