sgatev added inline comments.
================
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;
----------------
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.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:54
+ std::make_unique<WatchedLiteralsSolver>());
+ Environment Environment(AnalysisContext, FuncDecl);
+ UncheckedOptionalAccessModel Analysis(ASTCtx);
----------------
Call this `Env` to disambiguate from the name of the type.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:87
+ withInitializer(HasOptionalCallDescendant)))))
+ .bind("f"),
+ this);
----------------
Let's add a constant for this so that we can reuse it in
`UncheckedOptionalAccessCheck::check`.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:84
+ if (!BlockToOutputState ||
+ BlockToOutputState->size() <= Context->getCFG().getExit().getBlockID())
+ return;
----------------
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?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h:69
+ void swap(optional &rhs) noexcept;
+};
+} // namespace absl
----------------
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp:24
+
+void f2(const absl::optional<int> &opt) {
+ if (opt.has_value()) {
----------------
Call this `checked_access`?
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:39
+/// A matcher for the optional classes covered by this model.
+ast_matchers::DeclarationMatcher optionalClass();
----------------
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:40
+/// A matcher for the optional classes covered by this model.
+ast_matchers::DeclarationMatcher optionalClass();
+
----------------
This should be a class member, no? `clang::dataflow::optionalClass` seems
underspecified.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:237
+ // FIXME: include either the name of the optional (if applicable) or a source
+ // range of the access for easier intepretation of the result.
State.Lattice.getSourceLocations().insert(ObjectExpr->getBeginLoc());
----------------
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
