Page MenuHomePhabricator

Added AST matcher for ignoring elidable constructors
ClosedPublic

Authored by jvikstrom on Jun 11 2019, 10:24 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Jun 11 2019, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 10:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr added inline comments.Jun 11 2019, 10:40 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
6455 ↗(On Diff #204097)

80 columns

6455 ↗(On Diff #204097)

It would help if you described why a user would want to skip the elidable constructor using this matcher rather than hardcoding the presence of such an AST node.

6457 ↗(On Diff #204097)

I'd suggest to rephrase the example using the "Given:" format (see other comments around), and wrap the code to 80 columns.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
575 ↗(On Diff #204097)

Can you run these tests in both C++14 and C++17 modes?

628 ↗(On Diff #204097)

Need a space before the open brace, and indentation in the function body (in every test).

631 ↗(On Diff #204097)

Please clang-format.

jvikstrom updated this revision to Diff 204229.Jun 12 2019, 1:19 AM
  • Made ignoreElidable also ignore materializeTemporaryExpr and reformatted code
gribozavr added inline comments.Jun 12 2019, 1:28 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
6455 ↗(On Diff #204229)

"Matches expressions that match InnerMatcher that are possibly wrapped in an elidable constructor."

Then a blank line, and the rest of the explanation.

6458 ↗(On Diff #204229)

part of the AST in C++14 and earlier.

6460 ↗(On Diff #204229)

Therefore, to write a matcher that works in all language modes, the matcher has to skip elidable constructor AST nodes if they appear in the AST.

6467 ↗(On Diff #204229)

Can this sample be simplified? For example, B does not need to be a template (I think), and f() can be simplified to not call B twice.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
628 ↗(On Diff #204097)

Not done? I was talking about the code in the code snippet.

hokein added inline comments.Jun 12 2019, 2:33 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
6477 ↗(On Diff #204229)

Is the matcher only strict to move constructor? looks like the matcher implementation is for general constructors, just ignoringElidableConstructorCall? we may have elidable copy constructor.

6479 ↗(On Diff #204229)

nit: LLVM uses CamelCase style.

6486 ↗(On Diff #204229)

I think this return is unnecessary, since we will fall through at line 6489.

jvikstrom updated this revision to Diff 204244.Jun 12 2019, 3:07 AM
jvikstrom marked an inline comment as done.
  • Updated example and fixed edge case in ignoringElidableConstructorCall
jvikstrom updated this revision to Diff 204245.Jun 12 2019, 3:19 AM
  • Using CamelCase and also renamed ignoringElidableMoveConstructorCall to ignoringElidableConstructorCall
jvikstrom updated this revision to Diff 204269.Jun 12 2019, 5:51 AM
jvikstrom marked 3 inline comments as done.
  • Added match conditionally overload to control in what language standard a match should run in
jvikstrom updated this revision to Diff 204277.Jun 12 2019, 6:40 AM
jvikstrom marked 10 inline comments as done.
  • Fixed wrong formatting in test code
gribozavr added inline comments.Jun 12 2019, 7:00 AM
clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
600 ↗(On Diff #204277)

Please inline the variables that are used once.

clang/unittests/ASTMatchers/ASTMatchersTest.h
61 ↗(On Diff #204277)

Please drop the explicit values.

64 ↗(On Diff #204277)

2a

68 ↗(On Diff #204277)

"2a" (we don't know the year yet)

144 ↗(On Diff #204277)

80 columns.

154 ↗(On Diff #204277)

No need for semicolon.

157 ↗(On Diff #204277)

I'd say it is too clever, and suggest to write an explicit switch.

jvikstrom updated this revision to Diff 204282.Jun 12 2019, 7:20 AM
jvikstrom marked 7 inline comments as done.
  • Using switch for choosing language standard
gribozavr added inline comments.Jun 12 2019, 7:48 AM
clang/unittests/ASTMatchers/ASTMatchersTest.h
69 ↗(On Diff #204282)

Cxx2aOrLater?

(no need to uppercase things)

159 ↗(On Diff #204282)

The "orlater" variants should not appear here, please use "default: llvm_unreachable()".

jvikstrom marked an inline comment as done.
  • Added default to switch for language modes
  • Updated outdated docs
jvikstrom updated this revision to Diff 204443.Jun 13 2019, 1:16 AM
jvikstrom marked an inline comment as done.
  • Removed dependency on unordered map

looks most good to me, a few nits.

clang/include/clang/ASTMatchers/ASTMatchers.h
6488 ↗(On Diff #204442)

This return statement is not needed.

clang/unittests/ASTMatchers/ASTMatchersTest.h
16 ↗(On Diff #204442)

looks like the include is not used?

153 ↗(On Diff #204442)

nit: add a llvm_unreachable for default.

177 ↗(On Diff #204442)

nit: no {} needed for a single-statement body.

jvikstrom updated this revision to Diff 204447.Jun 13 2019, 1:27 AM
jvikstrom marked 3 inline comments as done.
  • Removed unnecessary return
hokein accepted this revision.Jun 13 2019, 2:18 AM

thanks, looks good to me. I'll commit for you. @gribozavr do you want to take another look on the patch?

clang/unittests/ASTMatchers/ASTMatchersTest.h
153 ↗(On Diff #204442)

this is not done?

This revision is now accepted and ready to land.Jun 13 2019, 2:18 AM
jvikstrom marked an inline comment as done.Jun 13 2019, 2:26 AM
jvikstrom added inline comments.
clang/unittests/ASTMatchers/ASTMatchersTest.h
153 ↗(On Diff #204442)

But the switch covers all enum values so that would give warnings

gribozavr accepted this revision.Jun 13 2019, 6:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 6:48 AM