Charusso added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:26
class CastValueChecker : public Checker<eval::Call> {
+ enum class CastKind { Checking, Sugar };
+
----------------
NoQ wrote:
> Please explain "Checking" and "Sugar". Checking what? Sugar... you mean
> syntactic sugar? For what?
I have no idea what devs mean by those names:
- `Checking` kind:
> The cast<> operator is a “checked cast” operation.
> The dyn_cast<> operator is a “checking cast” operation.
from
http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates
- `Sugar` kind:
> Member-template castAs<specific type>. Look through sugar for the underlying
> instance of <specific type>.
> Member-template getAs<specific type>'. Look through sugar for an instance of
> <specific type>.
from
https://clang.llvm.org/doxygen/classclang_1_1Type.html#a436b8b08ae7f2404b4712d37986194ce
and
https://clang.llvm.org/doxygen/classclang_1_1Type.html#adae68e1f4c85ede2d36da45fbefc48a2
- `isa()` would be `Instance` kind:
> The isa<> operator works exactly like the Java “instanceof” operator.
from
http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates
If you could imagine better names, please let me know. I have tried to use the
definitions.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:94
+ [CastFromName, CastToName, IsNullReturn,
+ IsSimpleCast](BugReport &) -> std::string {
SmallString<128> Msg;
----------------
NoQ wrote:
> `IsDynamicCast`.
It is not `dyn_cast` but simple `cast`. Because we have decided to use
`Checked` for that cast, I believe `IsCheckedCast` the one.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:98
- Out << "Assuming dynamic cast from '" << CastFromName << "' to '"
- << CastToName << "' succeeds";
- return Out.str();
- },
- /*IsPrunable=*/true);
+ Out << (!IsSimpleCast ? "Assuming dynamic cast " : "Dynamic cast ");
+ if (CastFromName)
----------------
NoQ wrote:
> More suggestions for the `:`-branch: "Hard cast" (a bit too jargon-y), "Safe
> cast", "Checked cast".
Well, let us pick `Checked` then as that is the proper definition for `cast()`,
but that other sugar-based `castAs()` definition makes me mad.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:220
- // If we cannot obtain both of the classes we cannot be sure how to model it.
- if (!CE->getType()->getPointeeCXXRecordDecl() ||
- !CE->getArg(0)->getType()->getPointeeCXXRecordDecl())
- return false;
+ const CastCheck *Check = &Lookup->first;
+ CastKind Kind = Lookup->second;
----------------
NoQ wrote:
> What's the point of making this a raw pointer? (no pun intended)
I just wanted to make it usable as it being used since the first review as
`(*Check)` and emphasize it is not a given function-call but a pointer to one.
If you think as a nude `Check()` it is fine, then I have no idea which is
better and it is fine for me as well.
================
Comment at: clang/test/Analysis/cast-value.cpp:156-167
+void evalNonNullParamNonNullReturn(const Shape *S) {
+ const auto *C = cast<Circle>(S);
+ // expected-note@-1 {{Dynamic cast from 'Shape' to 'Circle' succeeds}}
+ // expected-note@-2 {{Assuming pointer value is null}}
+ // expected-note@-3 {{'C' initialized here}}
+
+ (void)(1 / !(bool)C);
----------------
NoQ wrote:
> Mmm, wait a sec. That's a false positive. `cast<>` doesn't accept null
> pointers. We have `cast_or_null` for this.
This `Assuming pointer value is null` note is very random. I believe it is not
made by me and my code is fine, so I have printed a graph:
{F9759380}
Do you see any problem?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65889/new/
https://reviews.llvm.org/D65889
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
