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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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