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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 10 inline comments as done.
ymandel added a comment.

Thanks for the review!



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:39-45
+  using dataflow::ControlFlowContext;
+  using dataflow::DataflowAnalysisContext;
+  using dataflow::DataflowAnalysisState;
+  using dataflow::Environment;
+  using dataflow::UncheckedOptionalAccessModel;
+  using dataflow::WatchedLiteralsSolver;
+  using llvm::Expected;
----------------
sgatev wrote:
> Do we really need all these using declarations? There seems to be one reference for each of these types. I think we can simply qualify the references.
No, we don't need them, but IMO they clarify the code. We're really heavy with the types, given that `auto` is discouraged, so I think pulling these out improves readability in some critical places (lines 56 and 63). I figured I'd do all for consistency. I've removed those not related to lines 56 and 63, or used more than once. WDYT?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:84
+  if (!BlockToOutputState ||
+      BlockToOutputState->size() <= Context->getCFG().getExit().getBlockID())
+    return;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121120