[clang-tools-extra] r229552 - [clang-tidy] Fixed two wrong fix-it cases in misc-inefficient-algorithm checker.

[Alexander Kornienko]

Looks good! I don't known if there's a better way to figure out if the
comparers used are compatible.

I've seen one case where a weaker comparison is used in the
std::equal_range method than in the corresponding container, but this seems
to be rather suspicious on its own, so a // NOLINT with an explanation
wouldn't hurt in this case.

On Tue, Feb 17, 2015 at 10:45 PM, Gabor Horvath <xazax.hun at gmail.com> wrote:

> Author: xazax
> Date: Tue Feb 17 15:45:38 2015
> New Revision: 229552
>
> URL: http://llvm.org/viewvc/llvm-project?rev=229552&view=rev
> Log:
> [clang-tidy] Fixed two wrong fix-it cases in misc-inefficient-algorithm
> checker.
>
> Modified:
>     clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
>     clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp
>
> Modified:
> clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp?rev=229552&r1=229551&r2=229552&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
> (original)
> +++ clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
> Tue Feb 17 15:45:38 2015
> @@ -17,9 +17,18 @@ using namespace clang::ast_matchers;
>  namespace clang {
>  namespace tidy {
>
> +static bool areTypesCompatible(QualType Left, QualType Right) {
> +  if (const auto *LeftRefType = Left->getAs<ReferenceType>())
> +    Left = LeftRefType->getPointeeType();
> +  if (const auto *RightRefType = Right->getAs<ReferenceType>())
> +    Right = RightRefType->getPointeeType();
> +  return Left->getCanonicalTypeUnqualified() ==
> +         Right->getCanonicalTypeUnqualified();
> +}
> +
>  void InefficientAlgorithmCheck::registerMatchers(MatchFinder *Finder) {
>    const std::string Algorithms =
> -      "^::std::(find|count|equal_range|lower_blound|upper_bound)$";
> +      "^::std::(find|count|equal_range|lower_bound|upper_bound)$";
>    const auto ContainerMatcher = classTemplateSpecializationDecl(
>        matchesName("^::std::(unordered_)?(multi)?(set|map)$"));
>    const auto Matcher =
> @@ -57,6 +66,14 @@ void InefficientAlgorithmCheck::check(co
>    const llvm::StringRef IneffContName = IneffCont->getName();
>    const bool Unordered =
>        IneffContName.find("unordered") != llvm::StringRef::npos;
> +  const bool Maplike = IneffContName.find("map") != llvm::StringRef::npos;
> +
> +  // Store if the key type of the container is compatible with the value
> +  // that is searched for.
> +  QualType ValueType = AlgCall->getArg(2)->getType();
> +  QualType KeyType =
> +      IneffCont->getTemplateArgs()[0].getAsType().getCanonicalType();
> +  const bool CompatibleTypes = areTypesCompatible(KeyType, ValueType);
>
>    // Check if the comparison type for the algorithm and the container
> matches.
>    if (AlgCall->getNumArgs() == 4 && !Unordered) {
> @@ -87,7 +104,7 @@ void InefficientAlgorithmCheck::check(co
>    const auto *IneffContExpr =
> Result.Nodes.getNodeAs<Expr>("IneffContExpr");
>    FixItHint Hint;
>
> -  if (!AlgCall->getLocStart().isMacroID()) {
> +  if (!AlgCall->getLocStart().isMacroID() && !Maplike && CompatibleTypes)
> {
>      std::string ReplacementText =
>          (llvm::Twine(Lexer::getSourceText(
>
> CharSourceRange::getTokenRange(IneffContExpr->getSourceRange()),
>
> Modified:
> clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp?rev=229552&r1=229551&r2=229552&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp
> (original)
> +++ clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp
> Tue Feb 17 15:45:38 2015
> @@ -23,6 +23,19 @@ template <typename K, typename Cmp = les
>    iterator end() const;
>  };
>
> +struct other_iterator_type {};
> +
> +template <typename K, typename V, typename Cmp = less<K>> struct map {
> +  typedef other_iterator_type iterator;
> +  iterator find(const K &k);
> +  unsigned count(const K &k);
> +
> +  iterator begin();
> +  iterator end();
> +  iterator begin() const;
> +  iterator end() const;
> +};
> +
>  template <typename K> struct unordered_set : set<K> {};
>
>  template <typename K, typename Cmp = less<K>> struct multiset : set<K,
> Cmp> {};
> @@ -32,15 +45,17 @@ template <typename FwIt, typename K> FwI
>  template <typename FwIt, typename K, typename Cmp>
>  FwIt find(FwIt, FwIt, const K &, Cmp);
>
> -template <typename FwIt, typename Pred>
> -FwIt find_if(FwIt, FwIt, Pred);
> +template <typename FwIt, typename Pred> FwIt find_if(FwIt, FwIt, Pred);
>
>  template <typename FwIt, typename K> FwIt count(FwIt, FwIt, const K &);
>
>  template <typename FwIt, typename K> FwIt lower_bound(FwIt, FwIt, const K
> &);
> +
> +template <typename FwIt, typename K, typename Ord>
> +FwIt lower_bound(FwIt, FwIt, const K &, Ord);
>  }
>
> -#define FIND_IN_SET(x) find(x.begin(), x.end(), 10)
> +#define FIND_IN_SET(x) find(x.begin(), x.end(), 10)
>  // CHECK-FIXES: #define FIND_IN_SET(x) find(x.begin(), x.end(), 10)
>
>  template <typename T> void f(const T &t) {
> @@ -93,4 +108,26 @@ int main() {
>    find(us.begin(), us.end(), 10);
>    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call
> should be
>    // CHECK-FIXES: {{^  }}us.find(10);{{$}}
> +
> +  std::map<int, int> intmap;
> +  find(intmap.begin(), intmap.end(), 46);
> +  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call
> should be
> +  // CHECK-FIXES: {{^  }}find(intmap.begin(), intmap.end(), 46);{{$}}
> +}
> +
> +struct Value {
> +  int value;
> +};
> +
> +struct Ordering {
> +  bool operator()(const Value &lhs, const Value &rhs) const {
> +    return lhs.value < rhs.value;
> +  }
> +  bool operator()(int lhs, const Value &rhs) const { return lhs <
> rhs.value; }
> +};
> +
> +void g(std::set<Value, Ordering> container, int value) {
> +  lower_bound(container.begin(), container.end(), value, Ordering());
> +  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call
> should be
> +  // CHECK-FIXES: {{^  }}lower_bound(container.begin(), container.end(),
> value, Ordering());{{$}}
>  }
>
>
> _______________________________________________
> 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/20150223/e8e598b4/attachment.html>