xazax.hun added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:301
+ const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+ if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+ OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||
----------------
So it looks like `operator<<` is the only operator we are not supporting. I
think it might be good to include some test code to ensure that its use does
not suppress warnings. (Also OK, if you decide to deal with this in a follow-up
PR.)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:334
+ case OO_LessEqual:
+ State = State->assume((&RetVal)->castAs<DefinedOrUnknownSVal>(), true);
+ break;
----------------
I think in cases where we know that the result is `true` or `false`, the
`RetVal` should probably be a constant instead of a conjured symbol with an
assumption.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:341
+ case OO_Spaceship:
+ // TODO: What would be a good thing to do here?
+ default:
----------------
Instead of marking this unreachable, I'd suggest to just return a conjured
symbol for now. Usually, we should not use asserts to test user input.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:354
+
+ if (FirstPtr && !SecondPtr &&
+ State->assume(FirstPtr->castAs<DefinedOrUnknownSVal>(), false)) {
----------------
I am not sure if we actually need all this special casing. You could create an
`SVal` that represents a nullpointer constant and use `evalBinOp` with that
`SVal` when there is no symbol available.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:394
+ default:
+ llvm_unreachable("cannot reach here");
+ }
----------------
`cannot reach here` is redundant information. That is already encoded in
`llvm_unreachable`. I suggest including a message that tells the reader **why**
is it unreachable. In this case it could be `"unexpected overloaded operator
kind"`.
================
Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+ clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+ clang_analyzer_eval(ptr > ptr); // expected-warning{{FALSE}}
----------------
Putting tests like this on the same path can be risky. Tests might split the
execution path (maybe not now, but in the future). I think it might be more
future proof to have a large switch statement that switches on an unknown value
and put the tests in separate cases.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104616/new/
https://reviews.llvm.org/D104616
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
