ymandel marked an inline comment as done.
ymandel added a comment.
In D121120#3494427 <https://reviews.llvm.org/D121120#3494427>, @xazax.hun wrote:
> Overall this looks good to me. However, I think this might not use the full
> potential of the check itself. With the information that the dataflow
> framework have it could distinguish between **potentially** unsafe accesses
> and **provably** unsafe accesses depending on whether the `has_value`
> property is constrained to be false. From the user point of view, it would be
> nice to emit different warning messages for the above two cases. This can
> help to gradually introduce this check to a larger codebase and focus on the
> higher severity diagnostics first.
Agreed. I've filed this in our issue tracker (which I'm still planning to port
over to github...).
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:84
+ if (!BlockToOutputState ||
+ BlockToOutputState->size() <= Context->getCFG().getExit().getBlockID())
+ return;
----------------
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.
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
