[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