[PATCH] D49511: [Sema/Attribute] Check for noderef attribute
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 23 06:45:31 PDT 2018
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
More information about the cfe-commits
mailing list