This is an archive of the discontinued LLVM Phabricator instance.

[clang][modules] Add support for merging lifetime-extended temporaries
ClosedPublic

Authored by Tyker on Nov 13 2019, 9:51 AM.

Diff Detail

Event Timeline

Tyker created this revision.Nov 13 2019, 9:51 AM

Functionally, this looks good, thanks.

clang/include/clang/Serialization/ASTReader.h
555

Typos:

"[...] merging.
containg the [...]"

->

"[...] merging,
containing the [...]"

clang/lib/AST/TextNodeDumper.cpp
1349–1350

We shouldn't need this: the address of the declaration is dumped anyway by the infrastructure. (If you meant to dump the subexpression, I don't think that's what this does.)

Traversing from the LifetimeExtendedTemporaryDecl to its subexpression for dumping purposes should be done by ASTNodeTraverser (in include/clang/AST/ASTNodeTraverser.h).

clang/lib/Serialization/ASTReaderDecl.cpp
2566

* on the right, please.

2576

This case is essentially entirely different from the primary implementation of mergeMergeable. Consider adding an overload or explicit specialization for the LifetimeExtendedTemporaryDecl case instead; we don't need the branch on the dyn_cast result below, and we don't need the allowODRLikeMergeInC check above (because lifetime-extended temporaries only exist in C++).

2577

We shouldn't be dumping from here :)

2579

Prefer the normal container interface here -- insert or operator[] -- rather than the nonstandard extension FindAndConstruct.

Tyker updated this revision to Diff 229156.Nov 13 2019, 12:30 PM
Tyker marked 6 inline comments as done.

fixed most comments.

Tyker added inline comments.Nov 13 2019, 12:33 PM
clang/lib/AST/TextNodeDumper.cpp
1349–1350

I needed it during debugging and I thought i could be useful to others. but yes it is unreachable from -ast-dump

If you meant to dump the subexpression, I don't think that's what this does.

it dumps the value of the pointer which can be used to know which MaterializedTemporaryExpr it is associated with.

Tyker marked an inline comment as done.Nov 13 2019, 1:10 PM
Tyker added inline comments.
clang/lib/AST/TextNodeDumper.cpp
1349–1350

ill add the traversal in the previous patch and remove subexpr here because it is better

Tyker updated this revision to Diff 229176.Nov 13 2019, 1:25 PM

fixed comments

xbolva00 added inline comments.
clang/test/Modules/Inputs/merge-lifetime-extended-temporary/module.modulemap
15

Add newline here.

Dtto in other tests above.

Tyker updated this revision to Diff 230790.Nov 24 2019, 4:17 AM

rebased
added new lines.

rsmith accepted this revision.Nov 26 2019, 1:56 PM
This revision is now accepted and ready to land.Nov 26 2019, 1:56 PM
Tyker updated this revision to Diff 231589.Nov 30 2019, 7:37 AM
This revision was automatically updated to reflect the committed changes.