ymandel added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:84
+ if (!BlockToOutputState ||
+ BlockToOutputState->size() <= Context->getCFG().getExit().getBlockID())
+ return;
----------------
ymandel wrote:
> sgatev wrote:
> > xazax.hun wrote:
> > > ymandel wrote:
> > > > xazax.hun wrote:
> > > > > xazax.hun wrote:
> > > > > > ymandel wrote:
> > > > > > > xazax.hun wrote:
> > > > > > > > xazax.hun wrote:
> > > > > > > > > Could the size of the vector ever be wrong? Should this be an
> > > > > > > > > assert instead?
> > > > > > > > Whoops, after the update this comment is out of place, now it
> > > > > > > > supposed to be on line 60.
> > > > > > > Based on my reading, it is a rare, but possible condition.
> > > > > > > Basically, we need code where the exit block is unreachable,
> > > > > > > which I believe can happen in weird cases like:
> > > > > > >
> > > > > > > ```
> > > > > > > while(true) {...}
> > > > > > > ```
> > > > > > > https://godbolt.org/z/rfEnfaWTv -- notice the lack of
> > > > > > > predecessors for the exit block.
> > > > > > >
> > > > > > > See the code here, which follows the ordering of the blocks and
> > > > > > > doesn't force blocks to be processed:
> > > > > > >
> > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L337-L364
> > > > > > Interesting. Since we already have optionals in the vector, I
> > > > > > assumed we will always have matching size. I think we might want to
> > > > > > change this so there is only one way for the analysis to not
> > > > > > provide a state for a basic block to make this a bit less
> > > > > > confusing,
> > > > > Actually, in the linked code I see `
> > > > > BlockStates.resize(CFCtx.getCFG().size(), llvm::None);`. So I would
> > > > > expect the size to be always right with possibly some `None`s for the
> > > > > nodes that were not processed.
> > > > > Actually, in the linked code I see `
> > > > > BlockStates.resize(CFCtx.getCFG().size(), llvm::None);`. So I would
> > > > > expect the size to be always right with possibly some `None`s for the
> > > > > nodes that were not processed.
> > > > Ah, my mistake! I thought `resize` only allocated the space. #TIL
> > > >
> > > > Changed to an assert. Thanks.
> > > >
> > > But this discussion shed light on an interesting detail. If the exit
> > > block is unreachable, we will not diagnose the unsafe accesses. I wonder
> > > if this worth a FIXME.
> > The documentation of `runDataflowAnalysis` already hints that the size of
> > the vector matches the size of the CFG. Do you think we should make this
> > more clear?
> > But this discussion shed light on an interesting detail. If the exit block
> > is unreachable, we will not diagnose the unsafe accesses. I wonder if this
> > worth a FIXME.
>
> > The documentation of runDataflowAnalysis already hints that the size of the
> > vector matches the size of the CFG. Do you think we should make this more
> > clear?
>
> Yes to both. I'll send a separate patch on that header with the doc changes.
On second thought, the FIXME is for the check, not the framework, so I've added
it here. I'll still send the comment tweak separately.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121120/new/
https://reviews.llvm.org/D121120
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
