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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
