[cfe-commits] [Patch][Review Request] Improve Clang attribute system

[Delesley Hutchins]

A few comments are attached.  Looks good to me once those are fixed.

  -DeLesley

On Fri, Mar 2, 2012 at 3:52 PM, Michael Han <Michael.Han at autodesk.com> wrote:
> Ping. Any comments?
>
>
>
> Attach latest patch generated against r151945.
>
>
>
> Michael
>
>
>
> From: Michael Han
> Sent: Wednesday, February 22, 2012 3:49 PM
> To: cfe-commits at cs.uiuc.edu
> Subject: RE: [Patch][Review Request] Improve Clang attribute system
>
>
>
> Hi,
>
>
>
> Here is the updated patch which also generates the case statements for
> AttributeList::getKind, along with a fix to normalize the name of generated
> attribute enumerator.
>
>
>
> Please review, thanks!
>
>
>
> Michael
>
>
>
> From: Michael Han
> Sent: Tuesday, February 21, 2012 4:46 PM
> To: cfe-commits at cs.uiuc.edu
> Subject: [Patch][Review Request] Improve Clang attribute system
>
>
>
> Hi,
>
>
>
> I am starting working on a set of patches that aim at making the process of
> adding attribute extensions to Clang more automated, and less error prone.
> Currently, to add an attribute, besides modify the attribute TD file, one
> has to modify several entries in Sema, and such manual effort could be
> automated the same way attribute parsing is automated in parser via table
> gen. This will hopefully save a lot of boilerplate code, and make
> maintaining attributes easier.
>
>
>
> So, my plan is to try to kill the attribute code in Sema that looks like
> could be generated in table gen. I remember someone mentioned that
> ultimately the AttributeList should be removed also but I felt it is too
> aggressive for me so I am going the incremental approach by starting
> refactoring existing attributes code in Sema into table gen, with the hope
> once it is done we don’t need AttributeList anymore : )
>
>
>
> Here is the first patch that generate the attribute kind enumeration list
> from Attr.td. I noticed that some attributes in Attr.td does not have an
> entry in Sema AttributeList, for example, here is the list
>
>
>
> AlignMac68k
>
> AsmLabel
>
> Final
>
> MBlazeInterruptHandler
>
> MBlazeSaveVoliles
>
> MSP430Interrupt
>
> MaxFieldAlignment
>
> Override
>
>
>
> I added a bit field “SemaHandler” in Attr.td and this value is by default
> set to 1 since most of attributes described in Attr.td has entry in
> AttributeList. For the attributes in the above list, this field is set to 0
> so there would not be entry generated for them in AttributeList (though.. it
> seems no harm to generate the entries for them..)
>
>
>
> Also, some entries in AttributeList are not present in Attr.td, here is the
> list
>
> _address_space
>
> _base_check
>
> _cf_returns_autoreleased
>
> _ext_vector_type
>
> _mode
>
> _neon_polyvector_type
>
> _neon_vector_type
>
> _objc_gc
>
> _objc_ownership
>
> _opencl_image_access
>
> _vector_size,
>
>
>
> I simply hard code them in the enumeration list with the enumerators
> generated by table gen together. Also, since the enumerator names are
> generated automatically they may differ slightly in spelling comparing to
> some of hand coded enumerators, so I also updated several places to use new
> enumerator.
>
>
>
> Please review and let me know if this is the right direction to go. Then, I
> will start moving more code into table gen, like the AttributeList::getKind,
> and refactor common semantic checking code like argument checking into table
> gen too. Thanks!
>
>
>
> Cheers
>
> Michael
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
-------------- next part --------------

+void ClangAttrParsedAttrListEmitter::run(raw_ostream &OS) {
+  OS << "// This file is generated by TableGen. Do not edit.\n\n";
...
+    if (SemaHandler) {
+      std::vector<StringRef> Spellings =
+      getValueAsListOfStrings(Attr, "Spellings");

Indent above line by two spaces.  

...
+        // skip some non normalized attributes name which already have
+        // a normalized version in the spelling. For example, prefer 
+        // "cdecl" over "__cdecl" 
+        if (AttrName.startswith("__") && !AttrName.endswith("__"))
+          continue;

The implementation here works for cdecl, but may accidentally skip 
other (future) attributes.  You should skip a spelling if, and only if, 
the normalized version of that spelling has already been output.  


+void ClangAttrParsedAttrKindsEmitter::run(raw_ostream &OS) {
+  OS << "// This file is generated by TableGen. Do not edit.\n\n";
...
+       // normalize attribute name so __foo__ becomes foo
+       if (AttrName.startswith("__") && AttrName.endswith("__")) {
+          AttrName = AttrName.substr(2, AttrName.size() - 4);
+          Spelling = AttrName;
+       }
+
+       // normalize attribute name so __foo becomes foo
+       if (AttrName.startswith("__"))\
+         AttrName = AttrName.substr(2, AttrName.size());

I would like to see the code to normalize spellings factored out into 
a separate routine that is called by both AttrListEmitter and 
AttrKindsEmitter.