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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
