[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&id=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 *)&ptr. -// 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 &Out, 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(&b); -// } -// -// The note message will be "uninitialized field 'this->vptr->x'", even though -// void pointers can't be dereferenced. This shoul
[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 *)&ptr. -// 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 &Out, 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(&b); -// } -// -// The note message will be "uninitialized field 'this->vptr->x'", even tho