RedDocMD added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:141-163
+ auto bugReportOnGet = [&](const Expr *E) -> auto {
+ if (const auto *MCE = llvm::dyn_cast<CXXMemberCallExpr>(E)) {
+ const auto *Method = MCE->getMethodDecl();
+ const auto *Record = MCE->getRecordDecl();
+ if (Method->getName() == "get" &&
+ Record->getDeclContext()->isStdNamespace() &&
+ Record->getName() == "unique_ptr") {
----------------
vsavchenko wrote:
> Is there any reason it's not a method of `FindWhereConstrained`?
Not really.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179-180
+ // P.get()
+ if (const auto *InnerCastExpr =
+ llvm::dyn_cast<ImplicitCastExpr>(Sub)) {
+ Sub = InnerCastExpr->getSubExpr();
----------------
vsavchenko wrote:
> I think it's better to `IgnoreParensAndCasts` instead of manual traversal.
What is IgnoreParensAndCasts`? I didn't find it in the source code anywhere
(ripgrep, that is).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:227
+ if (const auto *DS = llvm::dyn_cast<DeclStmt>(S)) {
+ const Decl *D = DS->getSingleDecl();
+ if (const auto *VD = llvm::dyn_cast<VarDecl>(D)) {
----------------
vsavchenko wrote:
> So, situations like `int *a = nullptr, *b = smart.get();` are not supported?
No it works even in that case (I have added a test for that). It's got to do
with how the AST data structures are (`int *a = nullptr, *b = smart.get();` is
considered a single decl).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:229
+ if (const auto *VD = llvm::dyn_cast<VarDecl>(D)) {
+ for (const auto *I : PtrsTrackedAndConstrained) {
+ if (I->getName() == VD->getName()) {
----------------
vsavchenko wrote:
> `llvm::find_if`
Not sure if that'll work neatly since I actually need the return value of the
predicate function (the report).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:238-245
+ }
+ }
+ }
+ }
+ }
+ }
+ }
----------------
vsavchenko wrote:
> This level of nestedness is frowned upon. It is a good tell that the function
> should be refactored.
> The following code:
> ```
> if (cond1) {
> . . .
> if (cond2) {
> . . .
> if (cond3) {
> . . .
> }
> }
> }
> return nullptr;
> ```
> can be refactored into:
> ```
> if (!cond1)
> return nullptr;
>
> . . .
> if (!cond2)
> return nullptr;
>
> . . .
> if (!cond3)
> return nullptr;
>
> . . .
> ```
>
> It is easier to follow the logic if the function is composed in this manner
> because from the very beginning you know that `else` with more stuff is not
> going to follow.
Do you still think that's the case now? (After breaking it into functions).
I also think that for the sort of pattern matching we are doing in this patch,
the nested if's make more sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97183/new/
https://reviews.llvm.org/D97183
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
