[PATCH] D51417: [analyzer][UninitializedObjectChecker] Updated comments
This revision was automatically updated to reflect the committed changes.
Closed by commit rL342215: [analyzer][UninitializedObjectChecker] Updated
comments (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51417?vs=163061=165446#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51417
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -1,4 +1,4 @@
-//===- UninitializedPointer.cpp --*- C++ -*-==//
+//===- UninitializedPointee.cpp --*- C++ -*-==//
//
// The LLVM Compiler Infrastructure
//
@@ -90,9 +90,8 @@
// Utility function declarations.
-/// Returns whether T can be (transitively) dereferenced to a void pointer type
-/// (void*, void**, ...). The type of the region behind a void pointer isn't
-/// known, and thus FD can not be analyzed.
+/// Returns whether \p T can be (transitively) dereferenced to a void pointer
+/// type (void*, void**, ...).
static bool isVoidPointer(QualType T);
using DereferenceInfo = std::pair;
@@ -107,9 +106,7 @@
// Methods for FindUninitializedFields.
//===--===//
-// Note that pointers/references don't contain fields themselves, so in this
-// function we won't add anything to LocalChain.
-bool FindUninitializedFields::isPointerOrReferenceUninit(
+bool FindUninitializedFields::isDereferencableUninit(
const FieldRegion *FR, FieldChainInfo LocalChain) {
assert(isDereferencableType(FR->getDecl()->getType()) &&
@@ -222,12 +219,11 @@
while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
R = Tmp->getAs();
-
if (!R)
return None;
// We found a cyclic pointer, like int *ptr = (int *)
-// TODO: Report these fields too.
+// TODO: Should we report these fields too?
if (!VisitedRegions.insert(R).second)
return None;
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -94,7 +94,9 @@
};
/// Represents that the FieldNode that comes after this is declared in a base
-/// of the previous FieldNode.
+/// of the previous FieldNode. As such, this descendant doesn't wrap a
+/// FieldRegion, and is purely a tool to describe a relation between two other
+/// FieldRegion wrapping descendants.
class BaseClass final : public FieldNode {
const QualType BaseClassT;
@@ -296,7 +298,7 @@
}
if (isDereferencableType(T)) {
- if (isPointerOrReferenceUninit(FR, LocalChain))
+ if (isDereferencableUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
@@ -314,7 +316,8 @@
llvm_unreachable("All cases are handled!");
}
- // Checking bases.
+ // Checking bases. The checker will regard inherited data members as direct
+ // fields.
const auto *CXXRD = dyn_cast(RD);
if (!CXXRD)
return ContainsUninitField;
@@ -361,6 +364,9 @@
const FieldRegion *FieldChainInfo::getUninitRegion() const {
assert(!Chain.isEmpty() && "Empty fieldchain!");
+
+ // ImmutableList::getHead() isn't a const method, hence the not too nice
+ // implementation.
return (*Chain.begin()).getRegion();
}
@@ -375,31 +381,11 @@
/// Prints every element except the last to `Out`. Since ImmutableLists store
/// elements in reverse order, and have no reverse iterators, we use a
/// recursive function to print the fieldchain correctly. The last element in
-/// the chain is to be printed by `print`.
+/// the chain is to be printed by `FieldChainInfo::print`.
static void printTail(llvm::raw_ostream ,
const FieldChainInfo::FieldChainImpl *L);
-// TODO: This function constructs an incorrect string if a void pointer is a
-// part of the chain:
-//
-// struct B { int x; }
-//
-// struct A {
-// void *vptr;
-// A(void* vptr) : vptr(vptr) {}
-// };
-//
-// void f() {
-// B b;
-// A a();
-// }
-//
-// The note message will be "uninitialized field 'this->vptr->x'", even though
-// void pointers can't be dereferenced. This should be changed
[PATCH] D51417: [analyzer][UninitializedObjectChecker] Updated comments
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Comments always welcome! Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp:363-364 + + // ImmutableList::getHead() isn't a const method, hence the not too nice + // implementation. return (*Chain.begin()).getRegion(); Ugh, why is it not `const`? Repository: rC Clang https://reviews.llvm.org/D51417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51417: [analyzer][UninitializedObjectChecker] Updated comments
Szelethus added a comment. Polite ping ^-^ Repository: rC Clang https://reviews.llvm.org/D51417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51417: [analyzer][UninitializedObjectChecker] Updated comments
Szelethus added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp:377-397
-// TODO: This function constructs an incorrect string if a void pointer is a
-// part of the chain:
-//
-// struct B { int x; }
-//
-// struct A {
-// void *vptr;
Fixed in rC339653, but apparently I forgot to remove this.
Repository:
rC Clang
https://reviews.llvm.org/D51417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51417: [analyzer][UninitializedObjectChecker] Updated comments
Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet,
whisperity.
Some of the comments are incorrect, imprecise, or simply nonexistent. Since I
have a better grasp on how the analyzer works, it makes sense to update most of
them in a single swoop.
I tried not to flood the code with comments too much, this amount feels just
right to me.
Repository:
rC Clang
https://reviews.llvm.org/D51417
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -1,4 +1,4 @@
-//===- UninitializedPointer.cpp --*- C++ -*-==//
+//===- UninitializedPointee.cpp --*- C++ -*-==//
//
// The LLVM Compiler Infrastructure
//
@@ -90,9 +90,8 @@
// Utility function declarations.
-/// Returns whether T can be (transitively) dereferenced to a void pointer type
-/// (void*, void**, ...). The type of the region behind a void pointer isn't
-/// known, and thus FD can not be analyzed.
+/// Returns whether \p T can be (transitively) dereferenced to a void pointer
+/// type (void*, void**, ...).
static bool isVoidPointer(QualType T);
using DereferenceInfo = std::pair;
@@ -107,9 +106,7 @@
// Methods for FindUninitializedFields.
//===--===//
-// Note that pointers/references don't contain fields themselves, so in this
-// function we won't add anything to LocalChain.
-bool FindUninitializedFields::isPointerOrReferenceUninit(
+bool FindUninitializedFields::isDereferencableUninit(
const FieldRegion *FR, FieldChainInfo LocalChain) {
assert(isDereferencableType(FR->getDecl()->getType()) &&
@@ -224,12 +221,11 @@
while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
R = Tmp->getAs();
-
if (!R)
return None;
// We found a cyclic pointer, like int *ptr = (int *)
-// TODO: Report these fields too.
+// TODO: Should we report these fields too?
if (!VisitedRegions.insert(R).second)
return None;
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -94,7 +94,9 @@
};
/// Represents that the FieldNode that comes after this is declared in a base
-/// of the previous FieldNode.
+/// of the previous FieldNode. As such, this descendant doesn't wrap a
+/// FieldRegion, and is purely a tool to describe a relation between two other
+/// FieldRegion wrapping descendants.
class BaseClass final : public FieldNode {
const QualType BaseClassT;
@@ -291,7 +293,7 @@
}
if (isDereferencableType(T)) {
- if (isPointerOrReferenceUninit(FR, LocalChain))
+ if (isDereferencableUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
@@ -309,7 +311,8 @@
llvm_unreachable("All cases are handled!");
}
- // Checking bases.
+ // Checking bases. The checker will regard inherited data members as direct
+ // fields.
const auto *CXXRD = dyn_cast(RD);
if (!CXXRD)
return ContainsUninitField;
@@ -356,6 +359,9 @@
const FieldRegion *FieldChainInfo::getUninitRegion() const {
assert(!Chain.isEmpty() && "Empty fieldchain!");
+
+ // ImmutableList::getHead() isn't a const method, hence the not too nice
+ // implementation.
return (*Chain.begin()).getRegion();
}
@@ -370,31 +376,11 @@
/// Prints every element except the last to `Out`. Since ImmutableLists store
/// elements in reverse order, and have no reverse iterators, we use a
/// recursive function to print the fieldchain correctly. The last element in
-/// the chain is to be printed by `print`.
+/// the chain is to be printed by `FieldChainInfo::print`.
static void printTail(llvm::raw_ostream ,
const FieldChainInfo::FieldChainImpl *L);
-// TODO: This function constructs an incorrect string if a void pointer is a
-// part of the chain:
-//
-// struct B { int x; }
-//
-// struct A {
-// void *vptr;
-// A(void* vptr) : vptr(vptr) {}
-// };
-//
-// void f() {
-// B b;
-// A a();
-// }
-//
-// The note message will be "uninitialized field 'this->vptr->x'", even though
-//
