[PATCH] D23427: [Clang-tidy] Comparison Misuse
xazax.hun abandoned this revision. xazax.hun added a comment. For the first case ToT clang compiler gives a warning (-Wstring-compare), for the second case, it generates a compiler error (error: ordered comparison between pointer and zero). Note that, older versions of clang did not even give a warning for that case. It looks like this check no longer makes sense considering the current warnings and errors of clang top of tree. 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
Re: [PATCH] D23427: [Clang-tidy] Comparison Misuse
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
Re: [PATCH] D23427: [Clang-tidy] Comparison Misuse
hokein added inline comments.
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:31
@@ +30,3 @@
+ unless(anyOf(hasOperatorName("=="), hasOperatorName("!="))),
+ hasEitherOperand(ignoringImpCasts(gnuNullExpr(
+ .bind("compareToNull"),
We should use `nullPointerConstant()` here to cover more null cases.
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:39
@@ +38,3 @@
+ Result.Nodes.getNodeAs("charToLiteral");
+ if (CharToLiteral)
+diag(CharToLiteral->getOperatorLoc(),
The code can be simplified as follows:
```
if (const auto * a = Result.Nodes.getNodeAs("charToLiteral")) {
...
} else if (const auto *CompareToNull =
Result.Nodes.getNodeAs("compareToNull")) {
...
}
```
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:41
@@ +40,3 @@
+diag(CharToLiteral->getOperatorLoc(),
+ "char* is compared to a string literal");
+
I'm wondering if we can automatically fix this case. Use `strcmp()` function is
sufficient to fix it, IMO?
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:52
@@ +51,2 @@
+} // namespace clang
+
an extra blank line.
Comment at: clang-tidy/misc/ComparisonMisuseCheck.h:20
@@ +19,3 @@
+/// This checker reports errors related to the misuse of the comparison
operator.
+/// It should warn for the following cases:
+/// - strcmp,strncmp,memcmp misuse.
s/should warn/warns
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
Eugene.Zelenko wrote:
> Spaces after commas,
Seems like your check doesn't warn any usage about the strcmp family functions.
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:3
@@ +2,3 @@
+
+misc-comparison-misuse
+==
Please also update `docs/ReleaseNotes.rst`.
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:10
@@ +9,3 @@
+Case 1:
+ ``char*`` is compared to a string literal.
+
Does the case cover `char[]` ?
Comment at: test/clang-tidy/misc-comparison-misuse.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-comparison-misuse %t
+
Also clang-format this example.
Comment at: test/clang-tidy/misc-comparison-misuse.cpp:13
@@ +12,3 @@
+void test_null_to_pointer(int *p){
+ if (NULL>=p);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: comparison to nullptr
[misc-comparison-misuse]
Add `if (p <= NULL);` test case.
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
Re: [PATCH] D23427: [Clang-tidy] Comparison Misuse
Eugene.Zelenko added inline comments.
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:21
@@ +20,3 @@
+void ComparisonMisuseCheck::registerMatchers(MatchFinder *Finder) {
+
+ Finder->addMatcher(
Please remove this line.
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
Re: [PATCH] D23427: [Clang-tidy] Comparison Misuse
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
Please run Clang-format on newly added files. Test case is definitely needs it.
Comment at: clang-tidy/misc/ComparisonMisuseCheck.h:19
@@ +18,3 @@
+
+/// This checker reports errors related to the misuse of the comparison
operator.
+/// It should warn for the following cases:
Check, please.
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
Spaces after commas,
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:6
@@ +5,3 @@
+
+This checker reports errors related to the misuse of the comparison operator.
+It should warn for the following cases:
Check, please.
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:13
@@ +12,3 @@
+.. code-block::
+ bool isMyString(const char * my){
+return "mystring"==my;//error. comparing pointer to string literal
Clang-format examples, please.
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
[PATCH] D23427: [Clang-tidy] Comparison Misuse
falho created this revision.
falho added reviewers: xazax.hun, o.gyorgy, alexfh, aaron.ballman, etienneb,
hokein, Prazek.
falho added a subscriber: cfe-commits.
falho set the repository for this revision to rL LLVM.
This checker warns for the misuse of comparison operators
- char* is compared to a string literal
- inequality operator usage for NULL
Repository:
rL LLVM
https://reviews.llvm.org/D23427
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/ComparisonMisuseCheck.cpp
clang-tidy/misc/ComparisonMisuseCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-comparison-misuse.rst
test/clang-tidy/misc-comparison-misuse.cpp
Index: test/clang-tidy/misc-comparison-misuse.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-comparison-misuse.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy %s misc-comparison-misuse %t
+
+#define NULL __null
+
+bool test_pointer_to_literal(const char *my){
+ bool b = (my=="mystring");
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: char* is compared to a string literal [misc-comparison-misuse]
+ return "mystring"==my;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: char* is compared to a string literal [misc-comparison-misuse]
+}
+
+void test_null_to_pointer(int *p){
+ if (NULL>=p);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: comparison to nullptr [misc-comparison-misuse]
+
+ if (NULL==p);
+
+ if (NULL!=p);
+}
+
Index: docs/clang-tidy/checks/misc-comparison-misuse.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-comparison-misuse.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - misc-comparison-misuse
+
+misc-comparison-misuse
+==
+
+This checker reports errors related to the misuse of the comparison operator.
+It should warn for the following cases:
+
+Case 1:
+ ``char*`` is compared to a string literal.
+
+.. code-block::
+ bool isMyString(const char * my){
+return "mystring"==my;//error. comparing pointer to string literal
+ }
+
+
+Case 2:
+ Inequality operator usage for ``NULL``.
+
+.. code-block:: c++
+ void(int * p){
+ if (NULL>=p)//error, use only NULL==p, NULL!=p
+ }
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -55,6 +55,7 @@
misc-argument-comment
misc-assert-side-effect
misc-bool-pointer-implicit-conversion
+ misc-comparison-misuse
misc-dangling-handle
misc-definitions-in-headers
misc-fold-init-type
Index: clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -15,6 +15,7 @@
#include "MisplacedConstCheck.h"
#include "UnconventionalAssignOperatorCheck.h"
#include "BoolPointerImplicitConversionCheck.h"
+#include "ComparisonMisuseCheck.h"
#include "DanglingHandleCheck.h"
#include "DefinitionsInHeadersCheck.h"
#include "FoldInitTypeCheck.h"
@@ -68,6 +69,8 @@
"misc-unconventional-assign-operator");
CheckFactories.registerCheck(
"misc-bool-pointer-implicit-conversion");
+CheckFactories.registerCheck(
+"misc-comparison-misuse");
CheckFactories.registerCheck(
"misc-dangling-handle");
CheckFactories.registerCheck(
Index: clang-tidy/misc/ComparisonMisuseCheck.h
===
--- /dev/null
+++ clang-tidy/misc/ComparisonMisuseCheck.h
@@ -0,0 +1,39 @@
+//===--- ComparisonMisuseCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPARISON_MISUSE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPARISON_MISUSE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This checker reports errors related to the misuse of the comparison operator.
+/// It should warn for the following cases:
+/// - strcmp,strncmp,memcmp misuse.
+/// - char* is compared to a string literal
+/// - inequality operator usage for NULL
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-comparison-misuse.html
+class ComparisonMisuseCheck : public ClangTidyCheck {
+public:
+ ComparisonMisuseCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
