This is an archive of the discontinued LLVM Phabricator instance.

ASTImporter: expressions, pt.2
ClosedPublic

Authored by a.sidorin on Nov 4 2015, 2:34 AM.

Details

Summary

This patch implements some expression-related AST node import (patch #2).

ArrayTypeTraitExpr
ExpressionTraitExpr
OpaqueValueExpr
ArraySubscriptExpr
ExplicitCastExpr
ImplicitValueInitExpr
OffsetOfExpr
CXXThisExpr
CXXThrowExpr
CXXNoexceptExpr
CXXDefaultArgExpr
CXXScalarValueInitExpr
CXXBindTemporaryExpr
CXXTemporaryObjectExpr
MaterializeTemporaryExpr
ExprWithCleanups
StaticAssertDecl
FriendDecl
DecayedType

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin updated this revision to Diff 39175.Nov 4 2015, 2:34 AM
a.sidorin retitled this revision from to ASTImporter: expressions, pt.2.
a.sidorin updated this object.
a.sidorin added a reviewer: sepavloff.
a.sidorin set the repository for this revision to rL LLVM.
a.sidorin added a subscriber: cfe-commits.
sepavloff edited edge metadata.Nov 9 2015, 12:24 AM

The patch requires tests.
For instance, import of CXXBaseSpecifier can be tested along with static_cast: if a class indeed is a base, static_cast will be processed silent, otherwise a message must appear. ArraySubscript could be tested using constexpr function. MaterializeTemporaryExpr, CXXBindTemporaryExpr and CXXBindTemporaryExpr probably also can be tested with constexpr functions if temporary object class has constexpr constructor.

lib/AST/ASTImporter.cpp
5614 ↗(On Diff #39175)

Use getStartLoc instead of getExprLoc. It more clearly resolves to ArrayTypeTraitExpr ::Loc.

5628 ↗(On Diff #39175)

getExprLoc -> getStartLoc.

5642 ↗(On Diff #39175)

getExprLoc -> getLocation.

5769 ↗(On Diff #39175)

getExprLoc -> getOperatorLoc.

5859 ↗(On Diff #39175)

Is there any reason to use subscript instead of push_back? If no, push_back is more clear.

5890 ↗(On Diff #39175)

This code calculates parameter of type canThrowResult:

CanThrowResult V;
if (E->isValueDependent)
  V = CT_Dependent;
else
  V = E->getValue() ? CT_Can : CT_Cannot;

It looks more clear that the manipulations with created expression, doesn't it?

5909 ↗(On Diff #39175)

getExprLoc -> getThrowLoc.

5923 ↗(On Diff #39175)

getExprLoc -> getUsedLocation

5973 ↗(On Diff #39175)

getExprLoc -> getParenOrBraceRange

6002 ↗(On Diff #39175)

Should ManglingNumber get numbers associated with 'to' context?

a.sidorin updated this revision to Diff 56682.May 10 2016, 3:25 AM
a.sidorin updated this object.
a.sidorin added a reviewer: spyffe.
a.sidorin removed rL LLVM as the repository for this revision.
a.sidorin marked 7 inline comments as done.May 10 2016, 3:31 AM
a.sidorin added inline comments.
lib/AST/ASTImporter.cpp
5859 ↗(On Diff #56682)

I usually prefer allocation with required size at the start and avoid push_back() because it may lead to reallocations.

6002 ↗(On Diff #56682)

Hm, that's may be true, but I'm not sure. Added a FIXME.

spyffe requested changes to this revision.May 10 2016, 10:41 AM
spyffe edited edge metadata.

Overall I'm very enthusiastic about this and only have a few nitpicks. Thanks for the hard work!

lib/AST/ASTImporter.cpp
2364 ↗(On Diff #56682)

I would expect this to be checked for nullptr and this function to error out.

2367 ↗(On Diff #56682)

Should this be checked for nullptr as is done for other imports of TypeSourceInfo?

5517 ↗(On Diff #56682)

This makes me happy :)

6317 ↗(On Diff #56682)

You probably mean to use nullptr

This revision now requires changes to proceed.May 10 2016, 10:41 AM
a.sidorin updated this revision to Diff 56860.May 11 2016, 1:32 AM
a.sidorin edited edge metadata.

Add error checks; replace NULL with nullptr.

a.sidorin marked 4 inline comments as done.May 11 2016, 1:36 AM
a.sidorin added inline comments.
lib/AST/ASTImporter.cpp
5527 ↗(On Diff #56860)

Nice :)

a.sidorin updated this revision to Diff 57796.May 19 2016, 8:16 AM
a.sidorin edited edge metadata.
a.sidorin marked an inline comment as done.

Add some basic tests for ExpressionTraitExpr and ArrayTypeTraitExpr.

sepavloff added inline comments.Jun 1 2016, 12:22 PM
lib/AST/ASTImporter.cpp
2373 ↗(On Diff #57796)

Maybe else before this statement so that in the case of error ToInfo remained default initialized?

2502–2507 ↗(On Diff #57796)

Is there a reason to handle lexical context of StaticAssertDecl separately? I would say static_assert is always in the context where it is declared, no?

2518–2519 ↗(On Diff #57796)

Since C++17 static_assert may be used without message, so null pointer here is not an error.

3356 ↗(On Diff #57796)

variable -> declaration?

3363 ↗(On Diff #57796)

Maybe it is more natural to put this check into bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context, RecordDecl *D1, RecordDecl *D2)? If records are equivalent, existing decl can be used as a result of import, if not - entire record must be recreated anyway.

a.sidorin marked 5 inline comments as done.Jul 1 2016, 2:00 AM

Thank you for your comments! And sorry for late response after pinging.

lib/AST/ASTImporter.cpp
2518–2519 ↗(On Diff #57796)

I will modify the check, but may be we should check the language standard as well?

3363 ↗(On Diff #57796)

Sorry, it seems like I don't completely understand your idea. Won't IsStructurallyEquivalent() do an additional amount of work?

a.sidorin marked 2 inline comments as done.Jul 1 2016, 2:09 AM
a.sidorin added inline comments.
lib/AST/ASTImporter.cpp
3363 ↗(On Diff #57796)

And also, even if they are equivalent, we should search for the equivalent FriendDecl to return it.

a.sidorin updated this revision to Diff 62474.Jul 1 2016, 2:26 AM

Fix some issues pointed by Serge Pavlov.
Serge: putting a check of FriendDecls to IsStructurallyEquivalent() seems like a good idea for me, but we need some additional changes for it. CXXRecodeDecl::getFirstDecl() is private and StructuralEquivalenceContext() is located in an anonymous namespace so we cannot forward-declare it in DeclCXX.h to make it friend. We can move StructuralEquivalenceContext() out of anonymous namespace, however. What do you think?

I don't think this small improvement in Importer is worth invasive changes in other components.

Thanks, Serge. Is it OK to commit?

a.sidorin updated this revision to Diff 64663.Jul 20 2016, 5:00 AM

An attempt to fix unit test on Windows.
Serge, thank you! Could you please check this patch again?

a.sidorin updated this revision to Diff 64676.Jul 20 2016, 6:22 AM

Removed unneeded matcher.

With this patch unit tests pass on Windows as well.
I do not have formal authority to approve patches, but for me the patch is good enough to be accepted.

lib/AST/ASTImporter.cpp
3422 ↗(On Diff #64676)

Consider using unsigned instead of int to avoid MSVC warning about mix of signed and unsigned types.

a.sidorin updated this revision to Diff 64704.Jul 20 2016, 9:51 AM

Fix signed/unsigned mismatch warning in the loop condition.

NoQ added a subscriber: NoQ.Jul 20 2016, 1:41 PM

Serge, Sean, is this patch OK to commit?

sepavloff accepted this revision.Aug 23 2016, 9:13 AM
sepavloff edited edge metadata.

LGTM.

spyffe accepted this revision.Aug 23 2016, 10:22 AM
spyffe edited edge metadata.

Yes, I look forward to testing this in LLDB. Thanks for your hard work.

This revision is now accepted and ready to land.Aug 23 2016, 10:22 AM
khazem added a subscriber: khazem.Sep 18 2016, 5:06 PM
phosek added a subscriber: phosek.Sep 23 2016, 12:27 PM

I'm updating this patch so that it rebases cleanly onto master, as this patch hasn't been updated for a couple of months...

At the time of writing, one of Clang's tests is failing with this patch. Specifically, there is a segfault at line 130 of test/ASTMerge/Inputs/exprs3.cpp. I'm working on getting this fixed now.

khazem edited edge metadata.Sep 23 2016, 12:54 PM

Don't hurry. The segfault is fixed already and patch waits for approval: https://reviews.llvm.org/D24807. I'll update this fix in Monday.

a.sidorin updated this revision to Diff 72614.Sep 27 2016, 1:41 AM
a.sidorin updated this object.
a.sidorin added reviewers: ABataev, aaron.ballman.

Merge patch D24807 to both fix segmentation fault and provide a test for it.

ABataev added inline comments.Sep 27 2016, 2:18 AM
lib/AST/ASTImporter.cpp
3414 ↗(On Diff #72614)

CXXRecordDecl *RD -> auto *RD

5569 ↗(On Diff #72614)

No need for 'else' here, unconditional statement

5583 ↗(On Diff #72614)

Remove '/*TemplateArgs=*/' comment, it is not required anymore

a.sidorin updated this revision to Diff 72625.Sep 27 2016, 4:16 AM

Address review comments; add accidentally missed file.

aaron.ballman accepted this revision.Sep 27 2016, 8:34 AM
aaron.ballman edited edge metadata.

LGTM with two minor nits.

lib/AST/ASTImporter.cpp
5563–5564 ↗(On Diff #72625)

Any particular reason not to use a range-based for loop over template_arguments()?

6283 ↗(On Diff #72625)

Spurious newline.

This revision was automatically updated to reflect the committed changes.

Committed after Aaron's comment were addressed. Big thanks to all reviewers!