[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/AST/DeclPrinter.cpp:312
+          // printing on the left side for readbility.
+        else if (const VarDecl *VD = dyn_cast<VarDecl>(D);
+                   VD && VD->getInit() &&
----------------
giulianobelinassi wrote:
> erichkeane wrote:
> > While compiling this, I discovered that the lack of curley braces on the `else if (canPrintOnLeftSide(A))` means that this `else-if` is actually an else to THAT instead, despite its indention (which implies it should be associated with the `if (const FunctionDecl...`).
> > 
> > Which is it?  I presume the indenting is what you mean, but it hasn't been tested in a way that matters.  Can you add a test that validates the var-decl printing on the LHS ONLY happens when it 'can' print on the LHS?
> Sorry for the warning there. I will post a fixed version of this as soon as my build with `-Werror=all` completes.
> 
> Just to be clear, the intended code with braces is:
> ```
>       AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
>       if (mustPrintOnLeftSide(A))
>         // If we must always print on left side (e.g. declspec), then mark as
>         // so.
>         AttrLoc = AttrPrintLoc::Left;
>       else if (canPrintOnLeftSide(A)) {
>         // For functions with body defined we print the attributes on the left
>         // side so that GCC accept our dumps as well.
>         if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
>             FD && FD->isThisDeclarationADefinition())
>           // In case Decl is a function with a body, then attrs should be print
>           // on the left side.
>           AttrLoc = AttrPrintLoc::Left;
> 
>           // In case it is a variable declaration with a ctor, then allow
>           // printing on the left side for readbility.
>         else if (const VarDecl *VD = dyn_cast<VarDecl>(D);
>                    VD && VD->getInit() &&
>                    VD->getInitStyle() == VarDecl::CallInit)
>           AttrLoc = AttrPrintLoc::Left;
>       }
>       // Only print the side matches the user requested.
>       if ((Loc & AttrLoc) != AttrPrintLoc::None)
>         A->printPretty(Out, Policy);
> ```
> 
> As you can see, both `if (const FunctionDecl *FD` and `else if (const VarDecl *VD` are inside the  `if (canPrintOnLeftSide())`, and the `AttrLoc` variable is only set to `AttrPrintLoc::Left` when this function returns `true`. Hence unless there is a bug in `canPrintOnLeftSide` or `mustPrintOnLeftSide` (which I don't see any), I can't see a case where PrintOnLeft is false and it prints on the left side.
> 
> Now, I could be wrong on this, but IIRC the brackets on this is optional once the `else if` would match the preceding else-less if, which in this case would be `if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);`, which is what the indentation meant.
yep, you're right, it WAS already behaving how it was supposed to.  Committed.  Please keep an eye out for failed buildbots/etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141714