HazardyKnusperkeks reopened this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

In D115060#3219116 <https://reviews.llvm.org/D115060#3219116>, @curdeius wrote:

> @HazardyKnusperkeks, it *seems* as if this commit (or one of others indicated 
> on Changes tab of the link below) provoked failures in ASan. But it could be 
> something else.
> https://lab.llvm.org/buildbot/#/builders/5/builds/16734
> Could you have a look?

Okay, I don't have access to ASAN or so, I'm using MinGW on Windows. But I 
suspect it is the Assignment of the `PreviousLine` since this is not existent 
every time. 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115060

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to