samestep added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:66
+ SourceLocations Locs;
+ for (const CFGBlock *Block : Context->getCFG()) {
+ // Skip blocks that were not evaluated.
----------------
xazax.hun wrote:
> While doing yet another iteration after we reached the fixed point is a valid
> approach, many analyses have a monotonic property when it comes to emitting
> diagnostics. In this case, when the analysis discovered that an optional
> access is unsafe in one of the iterations I would not expect it to become
> safe in subsequent iterations. In most of the cases this makes collecting
> diagnostics eagerly a viable option and saves us from doing one more
> traversal of the CFG. On the other hand, we need to be careful to not to emit
> duplicate diagnostics.
>
> I wonder whether, from a user point of view, not having to do one more
> iteration is a more ergonomic interface.
@xazax.hun Agreed, I don't think for this particular analysis there's a risk of
diagnostics being prematurely added and then becoming invalid as the analysis
progresses. However, the broader goal of this patch is to separate the
information-gathering part of the analysis to the diagnostic-flagging part of
the analysis, with the hope that we can maybe derive the former automatically
while still allowing the latter to be written manually. It seems to me that it
would be infeasible to automatically derive both.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127898/new/
https://reviews.llvm.org/D127898
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
