[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

[Aaron Ballman via Phabricator via cfe-commits]

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
----------------
leonardchan wrote:
> aaron.ballman wrote:
> > 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`.
> `noderef` doesn't do anything with references or ObjC pointers for now at least. I was initially attempting to replicate the `noderef` logic from sparse's validation tests which provides examples in a C file which was my only intention for now.
> 
> I would be open to extending this to work with references or ObjC pointers if there's demand for it or there are examples of how it could be used with these types.
> noderef doesn't do anything with references or ObjC pointers for now at least. I was initially attempting to replicate the noderef logic from sparse's validation tests which provides examples in a C file which was my only intention for now.

I'm fine with that for the initial patch, but the documentation should clearly spell out that references and Objective-C pointer types are not supported, and there should be logic to diagnose cases where the attribute does is applied to a non-pointer type.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9420
+def warn_dereference_of_noderef_type : Warning<
+  "dereferencing %0; was declared with a `noderef` type">, InGroup<DiagGroup<"dereferenced-noderef">>;
 } // end of sema component.
----------------
The formatting looks off here, seems to be > 80 col.


================
Comment at: test/Frontend/noderef.c:2
+// RUN: %clang_cc1 -x c -verify %s
+// RUN: %clang_cc1 -x c++ -verify %s
+
----------------
leonardchan wrote:
> aaron.ballman wrote:
> > I would rather see a separate C++ test file that tests things like pointers to members, references, templates, etc.
> I probably shouldn't have included the `-x c++` in the first place since, as far as I know, usage of noderef in sparse is only in C which is mainly what we're trying to support for now. Though I wouldn't mind extending this to C++ once I find examples of usage of this in C++ or fully flesh out how this should be used with references, templates, etc.
Being primarily an attribute for C doesn't mean we should ignore the behavior of the attribute in C++ for testing purposes. This should have a C++ test file that tests the C++ spelling and ensures there are no surprises on C++ types that may be pointer-like (references, member pointers, etc). I think the same is true for Obj-C given that they have some odd pointer types. It's not that the attribute needs to be *supported* on those constructs, but we should have tests demonstrating what the actual behavior is expected to be for right now (e.g., ensuring it doesn't crash or assert, diagnoses as expected, etc).

This is also missing test cases ensuring the attribute appertains to the correct types, doesn't accept arguments, etc.


Repository:
  rC Clang

https://reviews.llvm.org/D49511