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

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi 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() &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141714