[cfe-commits] [PATCH] Add support for pretty-printing attributes

[Douglas Gregor]

On Nov 11, 2011, at 1:25 PM, Richard Membarth wrote:

> 
> On 2011.11.07 20:00, Douglas Gregor wrote:
>> 
>> On Nov 4, 2011, at 7:44 AM, Richard Membarth wrote:
>> 
>>> Attached is a patch that adds support for pretty-printing attributes
>>> stored in the AST. Tblgen is used to generate the necessary code for
>>> pretty printing attributes as suggest by Peter in [1].
>>> 
>>> Please let me know if this is ok.
>> 
>> 
>> This generally looks good. My only non-trivial comment is that this printing assumes that all of the attributes can be pretty-printed by just printing the arguments as a comma-separated list, but I doubt that's actually the case. If nothing else, the printing of VersionArguments looks incorrect for the "availability" attribute. It would be better to have some way to escape out to hand-written printing code, and then implement that code for the cases where the default comma-separated list printing doesn't work.
> 
> I looked into this today and updated the patch, which is attached to
> this mail. Printing of VersionArguments was indeed not correct. I
> added a check to call a separate hand-written pretty printing
> function for the "availability" attribute. The same can be done in
> case other attributes need a similar special handling.
> I hope this is now sufficient for pretty printing the currently
> available C/C++ attributes.
> 
>> 
>> One minor thing:
>> 
>> @@ -362,6 +382,18 @@ namespace {
>>          << getLowerName() << "_end(); i != e; ++i)\n";
>>       OS << "      " << WritePCHRecord(type, "(*i)");
>>     }
>> +    void writeValue(raw_ostream &OS) const {
>> +      OS << "\";\n";
>> +      OS << "  bool isFirst = true;\n"
>> +         << "  for (" << getAttrName() << "Attr::" << getLowerName()
>> +         << "_iterator i = " << getLowerName() << "_begin(), e = "
>> +         << getLowerName() << "_end(); i != e; ++i) {\n"
>> +         << "    if (!isFirst) isFirst = false;\n"
>> +         << "    else OS << \", \";\n"
>> 
>> Shouldn't this be if(isFirst)?
> 
> Yes, this is also updated in the patch.


Excellent, thanks! Committed as r145002.

	- Doug