This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix printing of lambdas with capture expressions
ClosedPublic

Authored by walrus on Jul 15 2020, 12:31 AM.

Diff Detail

Event Timeline

walrus created this revision.Jul 15 2020, 12:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 12:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
walrus retitled this revision from [clang] fix printing of lambdas with capture expressions to [clang] Fix printing of lambdas with capture expressions.Jul 15 2020, 12:48 AM
kadircet added inline comments.Jul 15 2020, 5:03 AM
clang/lib/AST/StmtPrinter.cpp
2011

what about having a Pre and Post print blocks, set to "(" and ")" or " = and ""` respectively?

that way we could get rid of the second if block below.

also i don't follow why blacklisting for parenlistexpr is needed here (i can see that it will end up printing double parens, but so is ParenExprs, and I think both of them shouldn't be showing up as initializer exprs of captured variables), could you elaborate with a comment and a test case?

walrus marked an inline comment as done.Jul 15 2020, 7:44 AM
walrus added inline comments.
clang/lib/AST/StmtPrinter.cpp
2011

I think you're right that skipping ParenListExpr is wrong here. I took this part of code from DeclPrinter. The ParenListExprs are skipped when printing variable declarations, but I think it is not applicable when printing lambda expression captures.

Honestly, I didn't get about Pre and Post blocks. I know it is supported when printing types, but I could not find how to do this for expressions.

kadircet added inline comments.Jul 15 2020, 8:26 AM
clang/lib/AST/StmtPrinter.cpp
2011

i meant something like below, (modulo naming):

llvm::StringRef Pre;
llvm::StringRef Post;
if(Style == CallInit) {
  Pre = "(";
  Post = ")";
}
else if (Style == CInit)
  Pre = " = ";

OS << Pre;
PrintExpr(Init):
OS << Post;
lattner resigned from this revision.Jul 15 2020, 8:56 AM
walrus updated this revision to Diff 278411.Jul 16 2020, 2:54 AM

Simplify code according to suggestions in code review

walrus updated this revision to Diff 278412.Jul 16 2020, 2:56 AM

Remove unused struct definition

walrus marked an inline comment as done.Jul 16 2020, 3:05 AM
walrus added inline comments.
clang/lib/AST/StmtPrinter.cpp
2011

Thank you for your suggestion - this makes code more clear. Regarding ParenListExpr exception - I investigated this case and found that this is necessary to prevent double braces when capturing variables of dependent unresolved types, e.g.:

template <typename T>
void foo(T var) {

  auto lambda = [m(var)] {};
}

Here expression m(var) resolves in ParenListExpr which will print braces on its own.

I added a couple of test cases to validate this behavior.

kadircet accepted this revision.Jul 16 2020, 3:20 AM

thanks, lgtm!

clang/lib/AST/StmtPrinter.cpp
2014

ah, now i see the use for it, thanks for the testcase !

could you please and with the previous condition rather than extra nesting.

2018

nit: braces

This revision is now accepted and ready to land.Jul 16 2020, 3:20 AM
walrus updated this revision to Diff 278420.Jul 16 2020, 3:33 AM

Simplify if..else statement, add braces

do you have commit access? if not I am happy to land this for you.

After having some commits done on your behalf, you can apply for commit access as described in https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

walrus marked 2 inline comments as done.Jul 16 2020, 3:40 AM

thanks, lgtm!

Thank you much for the review!

I do not have commit access. Could you please commit the changes on my behalf?
Ilya Golovenko <ilya.golovenko@huawei.com>

This revision was automatically updated to reflect the committed changes.