aaron.ballman added inline comments.
================ Comment at: include/clang/Basic/AttrDocs.td:3426 + let Content = [{ +The ``noderef`` attribute causes clang to throw a warning whenever a pointer marked with +this attribute is dereferenced. This is ideally used with pointers that point to special ---------------- The compiler doesn't "throw" warnings, so I would probably reword this in terms of "diagnoses" similar to what @erik.pilkington was suggesting. ================ Comment at: include/clang/Basic/AttrDocs.td:3426 + let Content = [{ +The ``noderef`` attribute causes clang to throw a warning whenever a pointer marked with +this attribute is dereferenced. This is ideally used with pointers that point to special ---------------- aaron.ballman wrote: > The compiler doesn't "throw" warnings, so I would probably reword this in > terms of "diagnoses" similar to what @erik.pilkington was suggesting. This leaves me wondering what happens with references in C++? Or Obj-C pointer types that aren't modeled as a `PointerType`. ================ Comment at: include/clang/Basic/DiagnosticGroups.td:1031 +// Warning group for warnings related to dereferencing of noderef pointers +def DereferencedNoDeref : DiagGroup<"dereferenced-noderef">; ---------------- There's no need for this diagnostic group to be spelled out here as there's only one diagnostic in the group. You can use `InGroup<"dereferenced-noderef">` directly in the diagnostic instead. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9420 +def warn_dereference_of_noderef_type : Warning< + "dereference of noderef expression">, InGroup<DereferencedNoDeref>; } // end of sema component. ---------------- The wording here isn't really accurate as the expression isn't what's marked `noderef`. Also, there may be multiple variables involved in the expression, so naming the variable would be useful. How about: `dereferencing %0; was declared with a `noderef` type` I'd also like to see a "declared here" note with the diagnostic. ================ Comment at: include/clang/Sema/Sema.h:987 - /// \brief Describes whether we are in an expression constext which we have + /// \brief Describes whether we are in an expression context which we have /// to handle differently. ---------------- This is unrelated to the patch, but feel free to commit the fix separately (no code review needed). ================ Comment at: include/clang/Sema/Sema.h:1027-1029 + void CheckSubscriptAccessOfNoDeref(const ArraySubscriptExpr *E); + void CheckAddressOfNoDeref(const Expr *E); + void CheckMemberAccessOfNoDeref(const MemberExpr *E); ---------------- I believe these can be private. ================ Comment at: include/clang/Sema/Sema.h:1037 + + static bool TypeHasNoDeref(const QualType &Ty); + ---------------- I believe this can be private. ================ Comment at: include/clang/Sema/Sema.h:1040-1041 +private: + unsigned NoDerefCallCount = 0; + std::unordered_set<const Expr *> PossibleDerefs; + ---------------- Rather than switch access control levels in the middle, can you move these with the other private data members? ================ Comment at: lib/Sema/SemaExpr.cpp:4168 +bool Sema::TypeHasNoDeref(const QualType &Ty) { + if (const AttributedType *AT = dyn_cast<AttributedType>(Ty)) { + AttributedType::Kind AttrKind = AT->getAttrKind(); ---------------- `const auto *` ================ Comment at: lib/Sema/SemaExpr.cpp:4259 + if (PossibleDerefs.find(StrippedExpr) != PossibleDerefs.end()) { + PossibleDerefs.erase(StrippedExpr); + } else if (const MemberExpr *Member = dyn_cast<MemberExpr>(StrippedExpr)) { ---------------- Rather than performing the lookup with `find()` and then performing the lookup a second time with `erase(<key>)`, it'd be better to just use the iterator from the call to `find()`. ================ Comment at: lib/Sema/SemaExpr.cpp:4260 + PossibleDerefs.erase(StrippedExpr); + } else if (const MemberExpr *Member = dyn_cast<MemberExpr>(StrippedExpr)) { + // Getting the address of a member expr in the form &(*s).b ---------------- `const auto *` ================ Comment at: lib/Sema/SemaExpr.cpp:4266 + PossibleDerefs.find(Base) != PossibleDerefs.end()) { + PossibleDerefs.erase(Base); + } ---------------- Similar suggestion here as above. ================ Comment at: lib/Sema/SemaExpr.cpp:4284 + QualType ElemTy; + if (const ArrayType *ArrayTy = dyn_cast<ArrayType>(BaseTy)) { + ElemTy = ArrayTy->getElementType(); ---------------- aaron.ballman wrote: > Can elide braces. `const auto *` ================ Comment at: lib/Sema/SemaExpr.cpp:4284-4291 + if (const ArrayType *ArrayTy = dyn_cast<ArrayType>(BaseTy)) { + ElemTy = ArrayTy->getElementType(); + } else if (const PointerType *PointerTy = dyn_cast<PointerType>(BaseTy)) { + ElemTy = PointerTy->getPointeeType(); + } else { + // Not a pointer access + return; ---------------- Can elide braces. ================ Comment at: lib/Sema/SemaExpr.cpp:4286 + ElemTy = ArrayTy->getElementType(); + } else if (const PointerType *PointerTy = dyn_cast<PointerType>(BaseTy)) { + ElemTy = PointerTy->getPointeeType(); ---------------- `const auto *` ================ Comment at: lib/Sema/SemaExpr.cpp:4293 + + if (const MemberExpr *Member = + dyn_cast<MemberExpr>(Base->IgnoreParenCasts())) { ---------------- `const auto *` ================ Comment at: lib/Sema/SemaExpr.cpp:4296 + const QualType &MemberBaseTy = Member->getBase()->getType(); + if (const PointerType *Ptr = dyn_cast<PointerType>(MemberBaseTy)) { + if (TypeHasNoDeref(Ptr->getPointeeType())) { ---------------- `const auto *` ================ Comment at: lib/Sema/SemaExpr.cpp:4297 + if (const PointerType *Ptr = dyn_cast<PointerType>(MemberBaseTy)) { + if (TypeHasNoDeref(Ptr->getPointeeType())) { + PossibleDerefs.insert(E); ---------------- Can elide braces. ================ Comment at: lib/Sema/SemaExprMember.cpp:1724 + + // Do not warn on member accesses to arrays + if (isa<ArrayType>(ResultTy)) ---------------- Missing full stop at the end of the sentence. ================ Comment at: lib/Sema/SemaExprMember.cpp:1729 + if (E->isArrow()) { + if (const PointerType *Ptr = + dyn_cast<PointerType>(E->getBase()->getType())) { ---------------- `const auto *` ================ Comment at: test/Frontend/noderef.c:2 +// RUN: %clang_cc1 -x c -verify %s +// RUN: %clang_cc1 -x c++ -verify %s + ---------------- I would rather see a separate C++ test file that tests things like pointers to members, references, templates, etc. Repository: rC Clang https://reviews.llvm.org/D49511 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits