[PATCH] D115060: [clang-format][NFC] Code Tidies in UnwrappedLineFormatter

Björn Schäpers via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 7 03:00:54 PST 2022


HazardyKnusperkeks added a comment.

In D115060#3224176 <https://reviews.llvm.org/D115060#3224176>, @owenpan wrote:

>> But I suspect it is the Assignment of the `PreviousLine` since this is not existent every time.
>
> Yep!
>
>> So I see the following solutions:
>>
>> 1. Only name `NextLine`, and use `I[-1]`.
>> 2. `const auto HasPreviousLine = I != AnnotatedLines.begin(); const auto &PreviousLine = HasPreviousLine ? *I[-1] : *I;` which is safe, since `PreviousLine` is only used if `HasPreviousLine` is true, but is a bit confusing. It would get an explaining comment.
>> 3. Rearrange the statements so that we can have only one check if there is a previous line and define `PreviousLine` inside that `if`. It remains to be seen if that's NFC.
>>
>> I would prefer option 3, but if that would change the behavior would go for option 2.
>
> There is option 4: change the type of `PreviousLine` to a pointer, and replace `PreviousLine.` with `PreviousLine->`. (You can rename `PreviousLine` if you think the naming is inconsistent with `NextLine`.) This would keep the patch being NFC.

I'm open for that, but I think the current diff (plus corrected formatting) is better. And at least according to our tests it is NFC.


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

https://reviews.llvm.org/D115060



More information about the cfe-commits mailing list