[PATCH] D121120: [clang-tidy] New check for safe usage of `std::optional` and like types.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun 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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121120