[cfe-dev] handle __attribute__((deprecated))

[Chris Lattner]

>>> BTW, I just saw how noreturn is handled (it enforces 0  
>>> parameters).  Do you prefer having this enforcement done in  
>>> deprecated as well as  other attributes?
>>
>> Yep, please.
>
> Ok, done in the patch.
>
> In the patch there's also code to handle nothrow+noreturn (including  
> codegen), partial format support (needs tweaking in the callexpr  
> sema), and adds a few more attributes that aren't used yet.
> Anyway, enough for today!

Wow, nice work Nuno!  Some minor critiques:

+class FormatAttr : public Attr {
+  std::string Type;
+  int formatIdx, firstArg;
+public:
+  FormatAttr(const std::string &type, int idx, int first) :  
Attr(Format), Type(type), formatIdx(idx), firstArg(first) {}

Please keep in 80 cols.

+/// DeclHasAttr - returns true if decl Declaration already has the  
target attribute
+static bool DeclHasAttr(const Decl *decl, const Attr *target) {

likewise.  Also, please end sentences with a period.  Here and in  
other comments.

+/// MergeAttributes - append attributes from the Old decl to the New  
one
+static void MergeAttributes(Decl *New, Decl *Old) {
+  for (const Attr *attr = Old->getAttrs(); attr; attr = attr- 
 >getNext()) {
+    if (!DeclHasAttr(New, attr)) {
+      New->addAttr(const_cast<Attr*>(attr));
+    }
+  }
+}

Doesn't this leave the old decl pointing to the attribute list?  This  
seems dangerous.  Please reset the attribute list on "old" to null.   
Also, if the attribute already exists, one of the two should be  
deleted (but be careful to set 'next' to null first, so that other  
attributes aren't deleted!).

+void Sema::HandleFormatAttribute(Decl *d, AttributeList *rawAttr) {
..
+  FunctionDecl *Fn = dyn_cast<FunctionDecl>(d);
+  if (!Fn) {
+    return;
+  }

Should this emit a diagnostic if it's not a function?  Ted, can you  
please review the rest of the 'HandleFormatAttribute' method?


+++ Driver/clang.cpp	(working copy)
@@ -865,6 +865,7 @@
      }

      AddPath("/usr/local/include", System, false, false, false,  
Headers);
+    AddPath("/usr/lib/gcc/i686-pc-linux-gnu/4.1.2/include", System,  
false, false, false, Headers); // nuno's laptop

Hopefully this isn't needed with the new CPATH envvar :)



+  if (FD->getAttr<NothrowAttr>())
+    ParamAttrsVec.push_back(
+     llvm::ParamAttrsWithIndex::get(ParamAttrsVec.size(),  
llvm::ParamAttr::NoUnwind));
+  if (FD->getAttr<NoReturnAttr>())
+    ParamAttrsVec.push_back(
+     llvm::ParamAttrsWithIndex::get(ParamAttrsVec.size(),  
llvm::ParamAttr::NoReturn));
+

More 80 col violations.  I think these attributes should always be  
applied to "0", the function itself, so the first arg should be 0, not  
ParamAttrsVec.size().  Try a function that is both noreturn and  
nounwind.

It would be nice to add "warn_unused_result" handling to -Wunused :),  
but that can clearly be a follow-on patch of course.

Incidentally, it would be nice if you could merge the test/CodeGen/ 
weak-function.c test/CodeGen/nothrow.c, etc testcases together.  While  
lots of little test cases is nice, if just one piece starts to fail,  
having larger tests that test multiple related things is useful  
because it makes the tests run faster.

Thanks again for working on this Nuno!

-Chris