Page MenuHomePhabricator

ASTImporter: expressions, pt.2

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



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


Diff Detail


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.

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;
  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.
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!

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.
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
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.

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.
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.

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.


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: 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
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.

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!