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

[Timo Stripf via Phabricator via cfe-commits]

strimo378 added a comment.

In D157394#4572777 <https://reviews.llvm.org/D157394#4572777>, @giulianobelinassi wrote:

> This patch do not address attributes in variables nor the __declspec case, as D141714 <https://reviews.llvm.org/D141714> do. His patch looks cleaner and I can surely coordinate with @strimo378 to also fix those cases, but I want an answer to the tblgen question first to see if we decide to drop the tblgen solution or not.

The patch is intended to only address function attributes to keep it simple and the test cases focused. Most important is that the AttrLocation is introduced and functionality can be easily extended to other decl types in subsequent patches. I did some work to address variable and record attributes but I think that I did not consider all cases for variables as you did.

`__declspec` should be always placed left?



================
Comment at: clang/lib/AST/DeclPrinter.cpp:259
+    case attr::Final:
+    case attr::Override:
+      return AttrLocation::AfterDecl;
----------------
giulianobelinassi wrote:
> 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?
> 
Currently, I would not go for a tblgen approach since we do not know all factors that influence the position. The current logic is relatively simple. I don't know how many additional exceptions are required.

I know for now that the position is influence by the target (function, record, field, parameter), attribute type and attribute syntax. To make matters worse, some attributes can be used for different targets. E.g. FinalAttr can be used for methods and records.

I suggest to keep things simple, stay on a function approach and see how complexity is evolving with future patches. Then we have the best knowledge to find a good tblgen solution.


================
Comment at: clang/lib/AST/DeclPrinter.cpp:259
+    case attr::Final:
+    case attr::Override:
+      return AttrLocation::AfterDecl;
----------------
strimo378 wrote:
> giulianobelinassi wrote:
> > 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?
> > 
> Currently, I would not go for a tblgen approach since we do not know all factors that influence the position. The current logic is relatively simple. I don't know how many additional exceptions are required.
> 
> I know for now that the position is influence by the target (function, record, field, parameter), attribute type and attribute syntax. To make matters worse, some attributes can be used for different targets. E.g. FinalAttr can be used for methods and records.
> 
> I suggest to keep things simple, stay on a function approach and see how complexity is evolving with future patches. Then we have the best knowledge to find a good tblgen solution.
> 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.

I also thought about that approach to decided on the location if it is placed before or after. I do a lot of AST modification where the location is lost or wrong. So for me that would not work in general.


================
Comment at: clang/lib/AST/DeclPrinter.cpp:267
+    // C2x/C++11-style attributes must appear before the declarator.
+    if (S == Attr::AS_CXX11 || S == Attr::AS_C2x)
+      return AttrLocation::BeforeDecl;
----------------
giulianobelinassi wrote:
> You can use A->isStandardAttributeSyntax()
Yes, ok


================
Comment at: clang/lib/AST/DeclPrinter.cpp:280
 
-void DeclPrinter::prettyPrintPragmas(Decl *D) {
+void DeclPrinter::prettyPrintAttributes(Decl *D, AttrLocation AL) {
   if (Policy.PolishForDeclaration)
----------------
giulianobelinassi wrote:
> I am pretty sure this function when called with AttrLocation::BeforeDecl will print an extra space before it prints out the attribute. Unfortunately to fix this it needs to be print into a string and then remove the first space from the generated string. Or do something more clever and patch printPretty so it does not do that.
Yes, you are fully right. The goal was to keep the patch simple.

To address the space problem, I think the best way would be to extend tblgen and add an optional bool parameter to printPritty function that allows suppressing the extra space. That could be addressed in a subsequent patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157394