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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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