Page MenuHomePhabricator

[ASTImporter] TypeAliasTemplate and PackExpansion importing capability
ClosedPublic

Authored by gerazo on Oct 24 2017, 10:42 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

gerazo created this revision.Oct 24 2017, 10:42 AM
gerazo updated this revision to Diff 121731.Nov 6 2017, 7:05 AM
gerazo added reviewers: spyffe, bruno.

ASTImporter unittests added.

xazax.hun added inline comments.Nov 13 2017, 3:09 AM
lib/AST/ASTImporter.cpp
1525 ↗(On Diff #121731)

The type should not be repeated here. Use auto.

1542 ↗(On Diff #121731)

The rest of the ASTImporter does not really follow this idiom (having asserts before most of the early returns.

unittests/AST/ASTImporterTest.cpp
492 ↗(On Diff #121731)

The formatting of the tests does not reflect the LLVM style, extra indent before struct. Same problem bellow.

a.sidorin edited edge metadata.Nov 13 2017, 4:47 AM

Hello Zoltan,

this code looks very familiar to me but it still needs some improvements. You can find my comments inline.

lib/AST/ASTImporter.cpp
775 ↗(On Diff #121731)

We should't assert here: in real code there can be conflicts and the return type can be null.

1542 ↗(On Diff #121731)

This looks like a copy-paste from our code on Github. Yes, we have catched tons of errors with them, but these asserts are not for production code.

1546 ↗(On Diff #121731)

cast<> will assert if import fails. You can use cast_or_null instead.

1547 ↗(On Diff #121731)

Another forgotten assert.

5257 ↗(On Diff #121731)

Another two asserts.

gerazo updated this revision to Diff 122643.Nov 13 2017, 6:19 AM

Removing needless asserts and typos

xazax.hun added inline comments.Nov 13 2017, 6:21 AM
lib/AST/ASTImporter.cpp
5254 ↗(On Diff #122643)

I think in LLVM most of the time we do not use braces for single statements guarded by conditions.
Also please mark comment that you addressed as done.

gerazo marked 8 inline comments as done.Nov 13 2017, 6:21 AM
gerazo updated this revision to Diff 122644.Nov 13 2017, 6:27 AM

removing non-conforming braces

gerazo marked an inline comment as done.Nov 13 2017, 6:27 AM
a.sidorin accepted this revision.Nov 13 2017, 9:09 AM

Looks good to me, thank you! Gabor?

This revision is now accepted and ready to land.Nov 13 2017, 9:09 AM
xazax.hun accepted this revision.Nov 13 2017, 9:10 AM

LGTM too!

Thank you guys!

This revision was automatically updated to reflect the committed changes.