Page MenuHomePhabricator

Added AST matcher for ignoring elidable constructors
ClosedPublic

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

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

80 columns

6455

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

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

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

628

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

631

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

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

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

6458

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

6460

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

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

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

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

nit: LLVM uses CamelCase style.

6486

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

Please inline the variables that are used once.

clang/unittests/ASTMatchers/ASTMatchersTest.h
61

Please drop the explicit values.

64

2a

68

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

144

80 columns.

154

No need for semicolon.

157

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
68

Cxx2aOrLater?

(no need to uppercase things)

158

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

This return statement is not needed.

clang/unittests/ASTMatchers/ASTMatchersTest.h
16

looks like the include is not used?

152

nit: add a llvm_unreachable for default.

176

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
152

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
152

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