[PATCH] D157394: [clang][DeclPrinter] Improve AST print of function attributes

Wed Aug 9 14:52:12 PDT 2023

giulianobelinassi added inline comments.

================
Comment at: clang/lib/AST/DeclPrinter.cpp:259
+    case attr::Final:
+    case attr::Override:
+      return AttrLocation::AfterDecl;
----------------
aaron.ballman wrote:
> giulianobelinassi wrote:
> > @aaron.ballman @erichkeane 
> > 
> > Is this enough to describe the position of the attribute kind, or should it go for a tblgen approach, as I did in D141714? This here looks much cleaner than the tblgen IMHO.
> Oofda, I forgot that we model those as keyword attributes. `override` and `final` (and other keyword attributes) are a bit trickier than just "before" or "after" because there's a very particular order in which those can be parsed grammatically, especially for functions: https://godbolt.org/z/ad5qecbrj
> 
> Thinking out loud: perhaps an approach to solving this is to sort the attributes by source location. So all the before attributes are written in the same order as they were in source, as are all of the after attributes. So long as we're skipping over implicit attributes, this seems like it should keep the order of the attributes correct.
> 
> A different alternative that's possibly more work than it's worth: the decl printer could dispatch to a table-genned function to ask the attribute "do you want to be printed now?" based on what part of the declaration we've already printed. This would require the decl printer to know e.g., "I've finished printing the parameter list but haven't yet printed cv qualifiers" and pass that information along to the attribute; individual attributes with special rules could supply special logic in Attr.td to say "the attribute should be printed after the function parameter list and after the cv and ref qualifiers, but before the override/final keywords" while most attributes can hopefully get away with whatever the default behavior is coming out of tablegen.
> Oofda, I forgot that we model those as keyword attributes. `override` and `final` (and other keyword attributes) are a bit trickier than just "before" or "after" because there's a very particular order in which those can be parsed grammatically, especially for functions: https://godbolt.org/z/ad5qecbrj
> 
> Thinking out loud: perhaps an approach to solving this is to sort the attributes by source location. So all the before attributes are written in the same order as they were in source, as are all of the after attributes. So long as we're skipping over implicit attributes, this seems like it should keep the order of the attributes correct.

This may work in cases where clang is able to find the SourceLocation of the Decl. My concern is when clang is *not* able to do it. When analyzing Linux sourcecode there are cases where this happen, and I have to fallback to AST dumping. The only way I see this working is by changing the parser for it to mark the attribute and its position. This may be something for the future, but the presented fix solves the issue for the cases we have already found. 

> A different alternative that's possibly more work than it's worth: the decl printer could dispatch to a table-genned function to ask the attribute "do you want to be printed now?" based on what part of the declaration we've already printed. This would require the decl printer to know e.g., "I've finished printing the parameter list but haven't yet printed cv qualifiers" and pass that information along to the attribute; individual attributes with special rules could supply special logic in Attr.td to say "the attribute should be printed after the function parameter list and after the cv and ref qualifiers, but before the override/final keywords" while most attributes can hopefully get away with whatever the default behavior is coming out of tablegen.

Sounds a lot of work :). Wouldn't my previous suggestion be simpler?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157394/new/

https://reviews.llvm.org/D157394