[cfe-commits] [PING^2] Handling of target attributes

[Chris Lattner]

On Jan 9, 2010, at 3:46 AM, Anton Korobeynikov wrote:

> Hello, Everyone
> 
> Almost all targets have their own attributes with different meaning.
> Think about e.g. "interrupt" attribute which might require different
> semantic checks & different codegen for every target.

This is great stuff, hopefully dllimport/export can be converted as well.

Some comments:

The MSP430InterruptAttr class should have at least one out of line virtual method, the clone one would make sense. I guess this is also true of all the other Attr classes.  Please do all of them as a follow-on patch or file a bugzilla about it :)

Please rename TargetABIInfo.cpp -> TargetInfo.cpp as a separate patch and commit that first.  Having it in this diff makes it impossible to see what you changed.

+++ b/lib/Sema/SemaDeclAttr.cpp

+    // Ask target about the attribute
+    const TargetAttributesSema& TargetAttrs = S.getTargetAttributesSema();

Plz end sentence with period, put & in the right place. :)


+++ b/lib/Sema/TargetAttributesSema.cpp

+namespace {
+  void HandleMSP430InterruptAttr(Decl *d, const AttributeList &Attr, Sema &S) {

Please make this a static function to avoid namespace nesting.  Just put MSP430AttributesSema in the namespace.


+    d->addAttr(::new (S.Context) MSP430InterruptAttr(Num));
+    d->addAttr(::new (S.Context) NoInlineAttr());
+    d->addAttr(::new (S.Context) UsedAttr());

It seems that you should only add the MSP430InterruptAttr attribute to the decl, but that codegen should add the other attributes to the LLVM IR function.  Is there a reason to add noinline and used to the AST?

Otherwise, the patch looks pretty good to me.  Please resent out another version of it with the file rename, so I can review the TargetABIInfo.cpp changes, thanks!

-Chris