This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
ClosedPublic

Authored by flx on May 11 2016, 7:49 AM.

Diff Detail

Event Timeline

flx updated this revision to Diff 56913.May 11 2016, 7:49 AM
flx retitled this revision from to [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
flx updated this object.
flx added reviewers: alexfh, sbenza.
flx set the repository for this revision to rL LLVM.
flx added a subscriber: cfe-commits.
aaron.ballman added inline comments.
clang-tidy/utils/TypeTraits.cpp
20

I would prefer this be used locally instead of at namespace scope to avoid potential name collisions.

33

!Record instead of explicit comparison.

35

Why use the matcher at all? You have the record decl, it can give you the copy constructor, and you can directly inspect whether that is deleted or not.

test/clang-tidy/performance-unnecessary-value-param.cpp
27

This is not a move-only type. Because you have a user-declared copy constructor (or destructor), the move constructor will not be implicitly declared. See [class.copy]p9 for details.

alexfh added inline comments.May 11 2016, 9:39 AM
clang-tidy/utils/TypeTraits.cpp
20

Note, that file-level using namespace ast_matchers is rather common in Clang tools, so I wouldn't bother here. That said, I'm fine either way.

flx updated this revision to Diff 57035.May 12 2016, 7:23 AM
flx removed rL LLVM as the repository for this revision.
flx marked 5 inline comments as done.
flx added inline comments.
clang-tidy/utils/TypeTraits.cpp
33

Does this make a difference for code generation or is this just a convention here for conciseness?

aaron.ballman added inline comments.May 12 2016, 7:32 AM
clang-tidy/utils/TypeTraits.cpp
31

No need to pass in Context any longer.

33

Should be no codegen difference in practice; it's just conciseness.

35

Can just use const auto * for this, but I'm fine either way.

alexfh added inline comments.May 12 2016, 9:22 AM
clang-tidy/utils/TypeTraits.cpp
22

Now seems to be unused.

etienneb added inline comments.
clang-tidy/utils/TypeTraits.cpp
35

I prefer const auto * too

44

nit: const ASTContext &Context

etienneb added inline comments.May 12 2016, 1:06 PM
clang-tidy/utils/TypeTraits.cpp
24

Should this be lifted to 'type_traits' ?

The same function exists into clang-tidy/utils/TypeTraits.cpp

namespace {
bool classHasTrivialCopyAndDestroy(QualType Type) {
  auto *Record = Type->getAsCXXRecordDecl();
  return Record && Record->hasDefinition() &&
         !Record->hasNonTrivialCopyConstructor() &&
         !Record->hasNonTrivialDestructor();
}
} // namespace

Alex, any toughs?

31

ditto, to be lifted or not?

flx updated this revision to Diff 57129.May 12 2016, 7:44 PM
flx marked 5 inline comments as done.
flx added inline comments.
clang-tidy/utils/TypeTraits.cpp
24

I'm not following. This is the same file.

31

What do you mean with lifted? Exposed in the header file?

44

isTriviallyCopyablType below takes non-const ASTContext.

etienneb added inline comments.May 12 2016, 8:18 PM
clang-tidy/utils/TypeTraits.cpp
24

My bad! I didn't saw the file change when reading. Forget about it.

31

ditto. I was thinking this was in the checker file.
ignore it.

etienneb added inline comments.May 12 2016, 9:53 PM
clang-tidy/utils/TypeTraits.cpp
44

You're right too.
But, it's possible to fix these prototypes:
http://reviews.llvm.org/D20226

alexfh accepted this revision.May 13 2016, 5:43 PM
alexfh edited edge metadata.

Looks good after addressing Etienne's comment.

Thanks!

This revision is now accepted and ready to land.May 13 2016, 5:43 PM
flx updated this revision to Diff 57286.May 14 2016, 3:38 PM
flx edited edge metadata.
flx marked 3 inline comments as done.
flx added inline comments.
clang-tidy/utils/TypeTraits.cpp
44

Updated to const reference now that your patch is in.

This revision was automatically updated to reflect the committed changes.