Page MenuHomePhabricator

[ASTImporter] Add unit tests for structural equivalence

Authored by martong on May 15 2018, 3:28 AM.



This patch add new tests for structural equivalence. For that a new common header is created which holds the test related language specific types and functions.

Event Timeline

martong created this revision.May 15 2018, 3:28 AM

Hello Gabor!

Thank you for this patch! Do you plan to enable this functionality for AST import checking?
Some comments are inline.

2 ↗(On Diff #148352)

Header line is too short.

9 ↗(On Diff #148352)

Could you please add a brief file description?

14 ↗(On Diff #148352)

System includes should follow LLVM includes.

38 ↗(On Diff #148352)

I think this function is too big for a header. Same below. Could you create a source file?

3 ↗(On Diff #148352)

Do we need ASTImporter.h?

40 ↗(On Diff #148352)


48 ↗(On Diff #148352)

Nit: comments should end with dot. Could you please check?

49 ↗(On Diff #148352)

Can we assert(FoundDecls.size() == 1)?

55 ↗(On Diff #148352)

D0, D1 (capital). Same below.

74 ↗(On Diff #148352)

Could we just overload testStructuralMatch to accept tuple or pair?

120 ↗(On Diff #148352)

Should we assert for spec_begin() != spec_end() instead or within these assertions?

164 ↗(On Diff #148352)

There is no Code1, so I think we can call it just "Code". Same below.

175 ↗(On Diff #148352)
  1. It's better to use more meaningful names than t. DeclTuple?
  2. The space after ( is redundant.
194 ↗(On Diff #148352)

Redundant space. Could you clang-format?

196 ↗(On Diff #148352)

These assertions are always true because there is a strong C assertion in the callee.

martong marked 14 inline comments as done.May 17 2018, 6:20 AM

Do you plan to enable this functionality for AST import checking?

Yes. We'd like to test the structural equivalency independently from ASTImporter, because in certain cases it may have faulty behavior. This can be very handy also when we further extend structural equivalency.

120 ↗(On Diff #148352)

I think that is not needed, because the previous cast in line 116 and 118 would assert if spec_begin() == spec_end() was true.

175 ↗(On Diff #148352)

Renamed t to Decls.

martong updated this revision to Diff 147304.May 17 2018, 6:21 AM
martong marked an inline comment as done.
  • Address aleksei's comments
a.sidorin accepted this revision.May 23 2018, 10:39 AM

Hi Gabor,

LGTM, thank you for addressing the comments! Just a minor nit, it's OK to fix it before committing without a separate review.

68 ↗(On Diff #148352)

Not sure we need this using: we can move up the using below or just write std::get twice.

This revision is now accepted and ready to land.May 23 2018, 10:39 AM
martong updated this revision to Diff 148352.May 24 2018, 1:42 AM

Moved using std::get up, before testStructuralMatch.

martong marked an inline comment as done.May 24 2018, 1:43 AM
This revision was automatically updated to reflect the committed changes.