<div dir="ltr"><div>+ // Pretty print pragma attribute. Attributes with pragma spellings should</div><div>+ // overload this method.</div><div><br></div><div>s/overload/override/</div><div><br></div><div>+ // FIXME: TableGen should be modified to print attributes with pragma</div>
<div>+ // spellings in a generic way.</div><div>+ virtual void printPrettyPragma(raw_ostream &OS,</div><div>+ const PrintingPolicy &Policy) const {</div><div>+ llvm_unreachable("printPrettyPragma must be specified for attributes with "</div>
<div>+ "a pragma spelling.");</div><div>+ }</div><div><br></div><div>Are you sure this needs to be a virtual method? I feel like we'd get better compile-time checks if we don't provide this for all Attr subclasses.</div>
<div><br></div><div>Don't you only emit calls to printPrettyPragma from the attribute class implementation if the attribute has a pragma spelling?</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, May 29, 2014 at 2:08 PM, Tyler Nowicki <span dir="ltr"><<a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Aaron,<div><br></div><div>Thanks for all the help with these patches. I’m learning a lot about clang.</div>
<div><br></div><div>Here is an updated patch. I did not add a prefix to the Pragma because I don’t really understand it too well, how to use it or test it. I used the printPrettyPragma approach described below.</div><div>
<br></div><div>If I missed any changes please let me know.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Tyler</div><div><br></div><div></div></font></span></div><br><div style="word-wrap:break-word">
<div></div><div><br><div><div>On May 28, 2014, at 11:59 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:</div><br><blockquote type="cite"><div style="font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
On Wed, May 28, 2014 at 2:45 PM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br><blockquote type="cite">On Wed, May 28, 2014 at 6:42 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
wrote:<br><blockquote type="cite"><br>On Tue, May 27, 2014 at 6:28 PM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br><blockquote type="cite">On Sat, May 24, 2014 at 8:28 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
wrote:<br><blockquote type="cite"><br><blockquote type="cite">My first idea is to add a tablegen property like "HasPrettyPrinter =<br>0<br>or 1"<br>that supresses emission of the default printPretty method. Then<br>
again,<br>we<br>kind of want the default version when the GNU and C++11 spellings of<br>the<br>attribute are used.<br></blockquote><br>That seems like the wrong way to go about it though, since this is a<br>new *spelling*, not a wholly new semantic kind of attribute (which I<br>
think we really want to avoid). We could possibly attach a custom<br>pretty printer to the spelling itself (a la a code block), but that<br>seems a bit skeezy because the arguments are rather important and the<br>spelling shouldn't have to care about the arguments.<br>
</blockquote><br><br>How about this: when generating the prettyPrint implementation, if there<br>is<br>a pragma spelling, call MyAttr::printPrettyPragma(...). Any attribute<br>with<br>a pragma spelling will have to provide this method in<br>
lib/AST/AttrImpl.cpp.<br></blockquote><br>So my ultimate goal is for pragma attributes to mirror all of the<br>other attributes, where we can infer syntax and semantics just from<br>what's in Attr.td. But it's becoming more obvious to me that the<br>
amount of work required to get there is rather large (mostly due to<br>the fact that pragma argument syntax is ALL over the place).<br><br>As a stopgap solution, it seems reasonable to me to rely on an<br>AdditionalMember being defined for pragmas (which is basically what<br>
you're suggesting).<br><br>So if an attribute has a pragma spelling, it is required to have:<br><br>let AdditionalMembers = [{<br> void printPrettyPragma(raw_ostream &OS, const PrintingPolicy &Policy)<br>const {<br>
}<br>}];<br></blockquote><br><br>Sure, but... I like writing C++ in .cpp files, not .td files. +1 for<br>defining it out of line in AttrImpl.cpp. :)<br></blockquote><br>That's an understandable desire, but a step in the wrong direction for<br>
two reasons. 1) Anything declarative that we can put into the .td file<br>belongs in the .td file. There's nothing about this problem which<br>isn't declarative aside from the fact that we don't have the time<br>
(yet) to make the proper declarations regarding parameter syntax. 2)<br>Putting it into AttrImpl.cpp decouples the logic from what gets<br>generated, which is far worse than writing some C++ code in a .td<br>file. You would have to write an out-of-line definition for<br>
FooAttr::printPrettyPragma where the declaration gets automatically<br>generated for you elsewhere, which is rather ugly as sin when we<br>already have an alternative that suffices.<br><br>Also, this isn't a whole lot of complex C++ code either. The less of<br>
it you write in the .td file, the more likely it is we will get the<br>declarative portions down without a mess of refactoring.<br><br>~Aaron</div></blockquote></div><br></div></div><br></blockquote></div><br></div>