aaron.ballman added a comment. Thank you for working on this check!
We already have a frontend diagnostic for comparisons between string literals and pointers, so I'm not certain of the utility of adding a clang-tidy check for that case (see -Wstring-compare, aka, http://coliru.stacked-crooked.com/a/6f6ca7fd2f6db09a). Comparisons against nullptr seems like it could also be handled as a frontend check as well, perhaps. ================ Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:23 @@ +22,3 @@ + Finder->addMatcher( + binaryOperator(hasEitherOperand(ignoringImpCasts(stringLiteral())), + hasEitherOperand(hasType(pointsTo(isAnyCharacter())))) ---------------- Should constrain this matcher to comparison operators, no? ================ Comment at: clang-tidy/misc/ComparisonMisuseCheck.h:21 @@ +20,3 @@ +/// It should warn for the following cases: +/// - strcmp,strncmp,memcmp misuse. +/// - char* is compared to a string literal ---------------- hokein wrote: > Eugene.Zelenko wrote: > > Spaces after commas, > Seems like your check doesn't warn any usage about the strcmp family > functions. Should remove the period at the end of this comment. Also, the w-variants for these APIs? I don't see any code related to strcmp and friends, so perhaps this comment is spurious and should be removed? ================ Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:1 @@ +1,2 @@ +.. title:: clang-tidy - misc-comparison-misuse + ---------------- The code examples in the documentation should be formatted with our usual style guidelines, such as `char *` rather than `char*`, etc. ================ Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:10 @@ +9,3 @@ +Case 1: + ``char*`` is compared to a string literal. + ---------------- hokein wrote: > Does the case cover `char[]` ? The check is covering any character type, including `char32_t`, `wchar_t`, etc. ================ Comment at: test/clang-tidy/misc-comparison-misuse.cpp:1 @@ +1,2 @@ +// RUN: %check_clang_tidy %s misc-comparison-misuse %t + ---------------- hokein wrote: > Also clang-format this example. Should be tests for other character types. Repository: rL LLVM https://reviews.llvm.org/D23427 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
