[clang-tools-extra] r234512 - [clang-tidy] Ignore expressions with incompatible deleters.

[David Blaikie]

On Thu, Apr 9, 2015 at 10:51 AM, Samuel Benzaquen <sbenza at google.com> wrote:

> Author: sbenza
> Date: Thu Apr  9 12:51:01 2015
> New Revision: 234512
>
> URL: http://llvm.org/viewvc/llvm-project?rev=234512&view=rev
> Log:
> [clang-tidy] Ignore expressions with incompatible deleters.
>
> Summary:
> Do not warn on .reset(.release()) expressions if the deleters are not
> compatible.
> Using plain assignment will probably not work.
>

Should we have a separate check for that, then? It seems error prone...


>
> Reviewers: klimek
>
> Subscribers: curdeius, cfe-commits
>
> Differential Revision: http://reviews.llvm.org/D8422
>
> Modified:
>     clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp
>
> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp
>
> Modified:
> clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp?rev=234512&r1=234511&r2=234512&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp
> (original)
> +++ clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp
> Thu Apr  9 12:51:01 2015
> @@ -22,17 +22,75 @@ void UniqueptrResetReleaseCheck::registe
>        memberCallExpr(
>            on(expr().bind("left")),
> callee(memberExpr().bind("reset_member")),
>            callee(methodDecl(hasName("reset"),
> -                            ofClass(hasName("::std::unique_ptr")))),
> +
> ofClass(recordDecl(hasName("::std::unique_ptr"),
> +
>  decl().bind("left_class"))))),
>            has(memberCallExpr(
>                on(expr().bind("right")),
>                callee(memberExpr().bind("release_member")),
> -              callee(methodDecl(hasName("release"),
> -                                ofClass(hasName("::std::unique_ptr")))))))
> +              callee(methodDecl(
> +                  hasName("release"),
> +                  ofClass(recordDecl(hasName("::std::unique_ptr"),
> +                                     decl().bind("right_class"))))))))
>            .bind("reset_call"),
>        this);
>  }
>
> +namespace {
> +const Type *getDeleterForUniquePtr(const MatchFinder::MatchResult &Result,
> +                                   StringRef ID) {
> +  const auto *Class =
> +      Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>(ID);
> +  if (!Class)
> +    return nullptr;
> +  auto DeleterArgument = Class->getTemplateArgs()[1];
> +  if (DeleterArgument.getKind() != TemplateArgument::Type)
> +    return nullptr;
> +  return DeleterArgument.getAsType().getTypePtr();
> +}
> +
> +bool areDeletersCompatible(const MatchFinder::MatchResult &Result) {
> +  const Type *LeftDeleterType = getDeleterForUniquePtr(Result,
> "left_class");
> +  const Type *RightDeleterType = getDeleterForUniquePtr(Result,
> "right_class");
> +
> +  if (LeftDeleterType->getUnqualifiedDesugaredType() ==
> +      RightDeleterType->getUnqualifiedDesugaredType()) {
> +    // Same type. We assume they are compatible.
> +    // This check handles the case where the deleters are function
> pointers.
> +    return true;
> +  }
> +
> +  const CXXRecordDecl *LeftDeleter =
> LeftDeleterType->getAsCXXRecordDecl();
> +  const CXXRecordDecl *RightDeleter =
> RightDeleterType->getAsCXXRecordDecl();
> +  if (!LeftDeleter || !RightDeleter)
> +    return false;
> +
> +  if (LeftDeleter->getCanonicalDecl() ==
> RightDeleter->getCanonicalDecl()) {
> +    // Same class. We assume they are compatible.
> +    return true;
> +  }
> +
> +  const auto *LeftAsTemplate =
> +      dyn_cast<ClassTemplateSpecializationDecl>(LeftDeleter);
> +  const auto *RightAsTemplate =
> +      dyn_cast<ClassTemplateSpecializationDecl>(RightDeleter);
> +  if (LeftAsTemplate && RightAsTemplate &&
> +      LeftAsTemplate->getSpecializedTemplate() ==
> +          RightAsTemplate->getSpecializedTemplate()) {
> +    // They are different instantiations of the same template. We assume
> they
> +    // are compatible.
> +    // This handles things like std::default_delete<Base> vs.
> +    // std::default_delete<Derived>.
> +    return true;
> +  }
> +  return false;
> +}
> +
> +}  // namespace
> +
>  void UniqueptrResetReleaseCheck::check(const MatchFinder::MatchResult
> &Result) {
> +  if (!areDeletersCompatible(Result))
> +    return;
> +
>    const auto *ResetMember =
> Result.Nodes.getNodeAs<MemberExpr>("reset_member");
>    const auto *ReleaseMember =
>        Result.Nodes.getNodeAs<MemberExpr>("release_member");
>
> Modified:
> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp?rev=234512&r1=234511&r2=234512&view=diff
>
> ==============================================================================
> ---
> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp
> (original)
> +++
> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp
> Thu Apr  9 12:51:01 2015
> @@ -2,12 +2,16 @@
>  // REQUIRES: shell
>
>  namespace std {
> +
>  template <typename T>
> +struct default_delete {};
> +
> +template <typename T, class Deleter = std::default_delete<T>>
>  struct unique_ptr {
> -  unique_ptr<T>();
> -  explicit unique_ptr<T>(T *);
> -  template <typename U>
> -  unique_ptr<T>(unique_ptr<U> &&);
> +  unique_ptr();
> +  explicit unique_ptr(T *);
> +  template <typename U, typename E>
> +  unique_ptr(unique_ptr<U, E> &&);
>    void reset(T *);
>    T *release();
>  };
> @@ -20,6 +24,9 @@ std::unique_ptr<Foo> Create();
>  std::unique_ptr<Foo> &Look();
>  std::unique_ptr<Foo> *Get();
>
> +using FooFunc = void (*)(Foo *);
> +using BarFunc = void (*)(Bar *);
> +
>  void f() {
>    std::unique_ptr<Foo> a, b;
>    std::unique_ptr<Bar> c;
> @@ -44,5 +51,20 @@ void f() {
>    Get()->reset(Get()->release());
>    // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 =
> std::move(ptr2)
>    // CHECK-FIXES: *Get() = std::move(*Get());
> +
> +  std::unique_ptr<Bar, FooFunc> func_a, func_b;
> +  func_a.reset(func_b.release());
> +  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 =
> std::move(ptr2)
> +  // CHECK-FIXES: func_a = std::move(func_b);
>  }
>
> +void negatives() {
> +  std::unique_ptr<Foo> src;
> +  struct OtherDeleter {};
> +  std::unique_ptr<Foo, OtherDeleter> dest;
> +  dest.reset(src.release());
> +
> +  std::unique_ptr<Bar, FooFunc> func_a;
> +  std::unique_ptr<Bar, BarFunc> func_b;
> +  func_a.reset(func_b.release());
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150409/a86c8ee6/attachment.html>