[PATCH] D158540: Improve error message for constexpr constructors of virtual base classes

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM, Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158540/new/

https://reviews.llvm.org/D158540

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158540: Improve error message for constexpr constructors of virtual base classes

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a subscriber: rsmith.
xgupta added a comment.

Just address @rsmith, I think after that we are fine to commit this review.

> +  "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class or 
> struct with virtual base classes">;

Please don't say "class or struct" here. Either just say "class" (a struct is a 
kind of class) or actually figure out which one it is and say that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158540/new/

https://reviews.llvm.org/D158540

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158540: Improve error message for constexpr constructors of virtual base classes

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D158540#4620286 , @NoumanAmir657 
wrote:

> @xgupta  It passed the test cases now

Thanks, I think we also want a note similar to MSVC diagnostic:

(6): note: see reference to function 'Derived::Derived(void)'


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158540/new/

https://reviews.llvm.org/D158540

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158540: Improve error message for constexpr constructors of virtual base classes

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added reviewers: shafik, aaron.ballman.
xgupta added a comment.

Test case updates are missing due to which 7 test cases are failing on 
pre-merge check - 
https://buildkite.com/llvm-project/premerge-checks/builds/172995#018a2776-1461-4f98-b12d-bd0521352d50/6-14972.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158540/new/

https://reviews.llvm.org/D158540

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D142609#4566418 , @aaron.ballman 
wrote:

> Concerns have been raised in 
> https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable 
> change in diagnostic behavior. The diagnostic is supposed to fire when 
> misusing a logical operand that most likely should have been a bitwise 
> operand. There's a feeling that `true && expr` is plausibly done 
> intentionally more often than `true & expr`.
>
> I think we should revert the changes from this patch and in the Clang 17.x 
> branch so that we can reevaluate the approach taken in this patch. CC 
> @porglezomp @cjdb

This got reverted and cherry-pick request is made - 
https://github.com/llvm/llvm-project/issues/64515.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157352: Revert "[Clang] Fix -Wconstant-logical-operand when LHS is a constant"

[Shivam Gupta via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa84525233776: Revert "[Clang] Fix 
-Wconstant-logical-operand when LHS is a constant" (authored by xgupta).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157352/new/

https://reviews.llvm.org/D157352

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,9 +43,7 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
-  // expected-note {{use '&' for a bitwise operation}} \
-  // expected-note {{remove constant to silence this warning}}
+  { return 2 && sizeof(OT); }
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,37 +76,15 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-
-  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
-  // cxx11-note@-1 {{use '&' for a bitwise operation}}
-  // cxx11-note@-2 {{remove constant to silence this warning}}
-  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
-  // cxx17-note@-4 {{use '&' for a bitwise operation}}
-  // cxx17-note@-5 {{remove constant to silence this warning}}
-
+  (0 && (a = 0)) + a; // ok
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
-  // cxx11-note@-3 {{use '&' for a bitwise operation}}
-  // cxx11-note@-4 {{remove constant to silence this warning}}
-  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
-  // cxx17-note@-6 {{use '&' for a bitwise operation}}
-  // cxx17-note@-7 {{remove constant to silence this warning}}
-
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
-  // cxx11-note@-3 {{use '|' for a bitwise operation}}
-  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
-  // cxx17-note@-5 {{use '|' for a bitwise operation}}
-  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
-  // cxx11-note@-1 {{use '|' for a bitwise operation}}
-  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
-  // cxx17-note@-3 {{use '|' for a bitwise operation}}
-
+  (1 || (a = 0)) + a; // ok
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,9 +44,6 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
-  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
-   // expected-note {{use '&' for a bitwise operation}} \
-   // expected-note {{remove constant to silence this warning}}
 
   return

[PATCH] D157352: Revert "[Clang] Fix -Wconstant-logical-operand when LHS is a constant"

[Shivam Gupta via Phabricator via cfe-commits]

xgupta created this revision.
Herald added a project: All.
xgupta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This reverts commit dfdfd306cfaf54fbc43e2d5eb36489dac3eb9976 
.

An issue is reported for wrong warning, this has to be reconsidered.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157352

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,9 +43,7 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
-  // expected-note {{use '&' for a bitwise operation}} \
-  // expected-note {{remove constant to silence this warning}}
+  { return 2 && sizeof(OT); }
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,37 +76,15 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-
-  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
-  // cxx11-note@-1 {{use '&' for a bitwise operation}}
-  // cxx11-note@-2 {{remove constant to silence this warning}}
-  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
-  // cxx17-note@-4 {{use '&' for a bitwise operation}}
-  // cxx17-note@-5 {{remove constant to silence this warning}}
-
+  (0 && (a = 0)) + a; // ok
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
-  // cxx11-note@-3 {{use '&' for a bitwise operation}}
-  // cxx11-note@-4 {{remove constant to silence this warning}}
-  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
-  // cxx17-note@-6 {{use '&' for a bitwise operation}}
-  // cxx17-note@-7 {{remove constant to silence this warning}}
-
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
-  // cxx11-note@-3 {{use '|' for a bitwise operation}}
-  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
-  // cxx17-note@-5 {{use '|' for a bitwise operation}}
-  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
-  // cxx11-note@-1 {{use '|' for a bitwise operation}}
-  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
-  // cxx17-note@-3 {{use '|' for a bitwise operation}}
-
+  (1 || (a = 0)) + a; // ok
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,9 +44,6 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
-  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
-   // expected-note {{use '&' for a bitwise operation}} \
-   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no

[PATCH] D147357: [clang-tidy] Add bugprone-optional-value-conversion check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM, Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147357/new/

https://reviews.llvm.org/D147357

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147357: [clang-tidy] Add bugprone-optional-value-conversion check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp:61
+
matchers::matchesAnyListedName(ValueMethods)
+  .bind("fun")),
+  hasType(qualType().bind("value-type")));

Any good reason to choose "fun" as the matcher name?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst:32
+}
+
+Value extraction using ``operator *`` is matched by default.

Can we also write - A better approach would be to directly pass opt to the 
print function without extracting its value: 
`print(opt)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147357/new/

https://reviews.llvm.org/D147357

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149015: [clang-tidy] Added bugprone-inc-dec-in-conditions check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149015/new/

https://reviews.llvm.org/D149015

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149015: [clang-tidy] Added bugprone-inc-dec-in-conditions check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp:30
+void IncDecInConditionsCheck::registerMatchers(MatchFinder *Finder) {
+  auto OperatorMacher = expr(
+  anyOf(binaryOperator(anyOf(isComparisonOperator(), isLogicalOperator())),

OperatorMacher  missing t in its spelling.



Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:141
 
+struct NotIdenticalStatementsPredicate {
+  bool

A comment can be here for this struct.





Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:150
+
+AST_MATCHER_P(Stmt, isStatementIdenticalToBoundNode, std::string, ID) {
+  NotIdenticalStatementsPredicate Predicate;

A comment can be here for this matcher.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst:62
+means that if ``i`` were initially ``5``, the first operand ``i < 5`` would
+evaluate to ``false`` and the second operand ``i > 2`` would not be evaluated.
+As a result, the decrement operation ``--i`` would not be executed and ``i``

If it is false then the second statement will be evaluated, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149015/new/

https://reviews.llvm.org/D149015

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146368: [clang-tidy] Add readability-reference-to-constructed-temporary check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146368/new/

https://reviews.llvm.org/D146368

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149084: [clang-tidy] Added bugprone-multi-level-implicit-pointer-conversion check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM, Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149084/new/

https://reviews.llvm.org/D149084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 544732.
xgupta added a comment.

minor update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,25 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types 
with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if (TautologicalTypeCompare &&
+  (LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical) &&
+  !S.Context.hasSameType(Constant->getType(), Other->getType())) {
+return false;
+  }
+
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,25 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if (TautologicalTypeCompare &&
+  (LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical) &&
+  !S.Context.hasSameType(Constant->getType(), Other->getType())) {
+return false;
+  }
+
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 544728.
xgupta added a comment.

updated code as per the comment of @cor3ntin


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp
  clang/test/SemaCXX/bool-compare.cpp


Index: clang/test/SemaCXX/bool-compare.cpp
===
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -8,13 +8,13 @@
   if(b > true){} // expected-warning {{comparison of true with expression 
of type 'bool' is always false}}
   if(b < true){} // no warning
   if(b >= true)   {} // no warning
-  if(b <= true)   {} // expected-warning {{comparison of true with expression 
of type 'bool' is always true}}
+  if(b <= true)   {} // expected-warning {{result of comparison of true with 
expression of type 'bool' is always true}}
   if(b == true)   {} // no warning
   if(b != true)   {} // no warning
 
   if(b > false)   {} // no warning
-  if(b < false)   {} // expected-warning {{comparison of false with expression 
of type 'bool' is always false}}
-  if(b >= false)  {} // expected-warning {{comparison of false with expression 
of type 'bool' is always true}}
+  if(b < false)   {} // expected-warning {{result of comparison of false with 
expression of type 'bool' is always false}}
+  if(b >= false)  {} // expected-warning {{result of comparison of false with 
expression of type 'bool' is always true}}
   if(b <= false)  {} // no warning
   if(b == false)  {} // no warning
   if(b != false)  {} // no warning
Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,25 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types 
with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if (TautologicalTypeCompare &&
+  (LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical) &&
+  !S.Context.hasSameType(Constant->getType(), Other->getType())) {
+return false;
+  }
+
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/SemaCXX/bool-compare.cpp
===
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -8,13 +8,13 @@
   if(b > true){} // expected-warning {{comparison of true with expression of type 'bool' is always false}}
   if(b < true){} // no warning
   if(b >= true)   {} // no warning
-  if(b <= true)   {} // expected-warning {{comparison of true with expression of type 'bool' is always true}}
+  if(b <= true)   {} // expected-warning {{result of comparison of true with expression of type 'bool' is always true}}
   if(b == true)   {} // no warning
   if(b != true)   {} // no warning
 
   if(b > false)   {} // no warning
-  if(b < false)   {} // expected-warning {{comparison of false with expression of type 'bool' is always false}}
-  if(b >= false)  {} // expected-warning {{comparison of false with expression of type 'bool' is always true}}
+  if(b < false)   {} // expected-warning {{result of comparison of false with expression of type 'bool' is always false}}
+  if(b >= false)  {} // expected-warning {{result of comparison of false with expression of type 'bool' is always true}}
   if(b <= false)  {} // no warning
   if(b == false)  {} // no warning
   if(b != false)  {} // no warning
Index: clang/test/Se

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4535941 , @aaron.ballman 
wrote:

> In D155457#4526168 , @xgupta wrote:
>
>> In D155457#4523388 , @cor3ntin 
>> wrote:
>>
>>> I'm not sure I understand the motivation for this change. Sure, people do 
>>> that but they also might do the same thing for ssize_t, intmax_t, or to 
>>> compare int to int32_t.
>>> I think a better heuristic would be to not emit a warning for any integral 
>>> (and floating point?) type that have the same canonical types (but we 
>>> probably still want one if their non-canonical type if the same)
>>
>> I am not sure but are you expecting these changes -
>>
>>   // Don't warn if the comparison involves integral or floating-point types 
>> with the same canonical types.
>>   QualType LHSCanonical = Constant->getType().getCanonicalType();
>>   QualType RHSCanonical = Other->getType().getCanonicalType();
>>   if ((LHSCanonical->isIntegralOrEnumerationType() || 
>> LHSCanonical->isFloatingType()) &&
>>   S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
>> return false;
>>   }
>>
>> This will silence a lot of warnings and a total 5 test case fails.
>
> Can you share some examples of what test cases start failing with that 
> approach? What you have above matches what I think @cor3ntin was asking for 
> and does seem like a pretty reasonable way to silence false positives.

Sure, updated three test cases in the patch and list the other two here -

  FAIL: Clang :: Sema/tautological-constant-compare.c (865 of 18988)
   TEST 'Clang :: Sema/tautological-constant-compare.c' 
FAILED 
  
  error: 'warning' diagnostics expected but not seen: 
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 560 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:568):
 comparison of 3-bit signed value < 4 is always true
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 574 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:585):
 comparison of 8-bit unsigned value < 0 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 593 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:595):
 comparison of 2-bit unsigned value > 3 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 603: result of comparison 'int' > 2147483647 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 608 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:610):
 comparison of 15-bit unsigned value > 32767 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 614 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:616):
 comparison of 6-bit signed value > 31 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 621 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:637):
 comparison of 4-bit signed value < -8 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 623 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:638):
 comparison of 4-bit signed value > 7 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 628 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:639):
 comparison of 5-bit signed value < -16 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 629 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:640):
 comparison of 5-bit signed value > 15 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 632 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:641):
 comparison of 4-bit signed value > 7 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 633 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:642):
 comparison of 4-bit signed value < -8 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 635 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:643):
 comparison of 5-bit si

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 544622.
xgupta added a comment.

Update as per comment of @cor3ntin


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/atomic-compare.c
  clang/test/Sema/tautological-objc-bool-compare.m
  clang/test/Sema/type-limit-compare.cpp
  clang/test/SemaCXX/bool-compare.cpp

Index: clang/test/SemaCXX/bool-compare.cpp
===
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -5,16 +5,16 @@
 
   bool a,b;
 
-  if(b > true){} // expected-warning {{comparison of true with expression of type 'bool' is always false}}
+  if(b > true){}
   if(b < true){} // no warning
   if(b >= true)   {} // no warning
-  if(b <= true)   {} // expected-warning {{comparison of true with expression of type 'bool' is always true}}
+  if(b <= true)   {}
   if(b == true)   {} // no warning
   if(b != true)   {} // no warning
 
   if(b > false)   {} // no warning
-  if(b < false)   {} // expected-warning {{comparison of false with expression of type 'bool' is always false}}
-  if(b >= false)  {} // expected-warning {{comparison of false with expression of type 'bool' is always true}}
+  if(b < false)   {}
+  if(b >= false)  {}
   if(b <= false)  {} // no warning
   if(b == false)  {} // no warning
   if(b != false)  {} // no warning
Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/test/Sema/tautological-objc-bool-compare.m
===
--- clang/test/Sema/tautological-objc-bool-compare.m
+++ clang/test/Sema/tautological-objc-bool-compare.m
@@ -15,10 +15,10 @@
   r = B >= 0; // expected-warning {{result of comparison of constant 0 with expression of type 'BOOL' is always true, as the only well defined values for 'BOOL' are YES and NO}}
   r = B <= 0;
 
-  r = B > YES; // expected-warning {{result of comparison of constant YES with expression of type 'BOOL' is always false, as the only well defined values for 'BOOL' are YES and NO}}
+  r = B > YES;
   r = B > NO;
-  r = B < NO; // expected-warning {{result of comparison of constant NO with expression of type 'BOOL' is always false, as the only well defined values for 'BOOL' are YES and NO}}
+  r = B < NO;
   r = B < YES;
-  r = B >= NO; // expected-warning {{result of comparison of constant NO with expression of type 'BOOL' is always true, as the only well defined values for 'BOOL' are YES and NO}}
+  r = B >= NO;
   r = B <= NO;
 }
Index: clang/test/Sema/atomic-compare.c
===
--- clang/test/Sema/atomic-compare.c
+++ clang/test/Sema/atomic-compare.c
@@ -14,10 +14,10 @@
   if (a > 2) {} // no warning
 
   if (!a > 0) {}  // no warning
-  if (!a > 1) {} // expected-warning {{comparison of constant 1 with boolean expression is always false}}
-  if (!a > 2) {} // expected-warning {{comparison of constant 2 with boolean expression is always false}}
+  if (!a > 1) {}
+  if (!a > 2) {}
   if (!a > b) {} // no warning
-  if (!a > -1){} // expected-warning {{comparison of constant -1 with boolean expression is always true}}
+  if (!a > -1){}
 }
 
 typedef _Atomic(int) Ty;
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,16 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if ((LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146368: [clang-tidy] Add readability-reference-to-constructed-temporary check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

This LGTM, I will wait for two days before accepting the revision in case other 
reviewers might have some more comments/suggestions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146368/new/

https://reviews.llvm.org/D146368

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146368: [clang-tidy] Add readability-reference-to-constructed-temporary check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst:18
+
+   const std::string& value("hello");
+

PiotrZSL wrote:
> xgupta wrote:
> > The below comment is not matching,  do you want to write - 
> > `const std::string& str = std::string("hello");`
> > ?
> actually comment is +- fine, constructor to std::string will be called 
> anyway, and for compiler I think those 2 are equal.
> Will verify.
but you have written below `reference variable ``str`` is assigned a temporary 
object` and there is no str variable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146368/new/

https://reviews.llvm.org/D146368

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146368: [clang-tidy] Add readability-reference-to-constructed-temporary check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp:20
+
+struct NotExtendedByDeclBoundToPredicate {
+  bool operator()(const internal::BoundNodesMap &Nodes) const {

A comment might be good to describe this struct.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp:22
+  bool operator()(const internal::BoundNodesMap &Nodes) const {
+const auto *Other = Nodes.getNode(ID).get();
+if (!Other)

This can be written as `const auto *Other = Nodes.getNodeAs(ID);` 
following other patterns.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp:39
+  ID) {
+  NotExtendedByDeclBoundToPredicate Predicate;
+  Predicate.ID = ID;

Can be written using direct initialization as  
`NotExtendedByDeclBoundToPredicate Predicate{ID};`



Comment at: 
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp:63
+
+bool ReferenceToConstructedTemporaryCheck::isLanguageVersionSupported(
+const LangOptions &LangOpts) const {

This part looks like boilerplate in the middle of a specific implementation., 
`isLanguageVersionSupported` is mostly in the header file.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp:69
+std::optional
+ReferenceToConstructedTemporaryCheck::getCheckTraversalKind() const {
+  return TK_AsIs;

This part look like boilerplate in the middle of a specific implementation., 
getCheckTraversalKind() is mostly in the header file.





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst:10
+This construction is often the result of multiple code refactorings or a lack
+of developer knowledge, leading to confusion or subtle bugs. In most cases,
+what the developer really wanted to do is create a new variable rather than

May be good to mention dangling references and resource leakage as potential 
issues.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst:18
+
+   const std::string& value("hello");
+

The below comment is not matching,  do you want to write - 
`const std::string& str = std::string("hello");`
?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146368/new/

https://reviews.llvm.org/D146368

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4523388 , @cor3ntin wrote:

> I'm not sure I understand the motivation for this change. Sure, people do 
> that but they also might do the same thing for ssize_t, intmax_t, or to 
> compare int to int32_t.
> I think a better heuristic would be to not emit a warning for any integral 
> (and floating point?) type that have the same canonical types (but we 
> probably still want one if their non-canonical type if the same)

I am not sure but are you expecting these changes -

  // Don't warn if the comparison involves integral or floating-point types 
with the same canonical types.
  QualType LHSCanonical = Constant->getType().getCanonicalType();
  QualType RHSCanonical = Other->getType().getCanonicalType();
  if ((LHSCanonical->isIntegralOrEnumerationType() || 
LHSCanonical->isFloatingType()) &&
  S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
return false;
  }

This will silence a lot of warnings and a total 5 test case fails.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4523227 , @aaron.ballman 
wrote:

> In D155457#4513812 , @xgupta wrote:
>
>> In D155457#4511652 , 
>> @aaron.ballman wrote:
>>
>>> In the x86 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is true, 
>>> so we test the other expression; because `Size` is greater than 
>>> `__SIZE_MAX__` the function returns false and the second static assertion 
>>> fails as expected.
>>> In the x64 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is false 
>>> (first static assertion fails) so we shouldn't even be evaluating the RHS 
>>> of the `&&` to see if it's tautological because it can't contribute to the 
>>> expression result, right?
>>
>> Yes, I agree with both statements but what is odd in current behavior?
>
> It's a false positive -- the tautological bit is diagnosed but it doesn't 
> contribute to the result of the expression. The first part of the predicate 
> is specifically intended to ensure the second part of the predicate is *not* 
> tautological.

Ok, Is there any modification required in this patch or it fixes that false 
positive?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97567: [clang-tidy] performance-* checks: Also allow allow member expressions to be used in a const manner.

[Shivam Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e512688376c: [clang-tidy] performance-* checks: Also allow 
allow member expressions to be… (authored by shivam-amd).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97567/new/

https://reviews.llvm.org/D97567

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
  clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -114,8 +114,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -140,8 +140,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -172,8 +172,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -199,8 +199,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
@@ -843,3 +843,36 @@
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+auto Copy = Orig;
+Copy.Member.nonConstMethod();
+  }
+  {
+auto Copy = Orig;
+mutate(Copy.Member);
+  }
+  {
+auto Copy = Orig;
+mutate(&Copy.Member);
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,38 @@
 // SS : createView(*ValueReturningIterator())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+// CHECK-FIXES: for (const Struct& SS : View>()) {
+auto MemberCopy = SS.Member;
+const auto &ConstRef = SS.Member;
+bool b = SS.Member.constMethod();
+use(SS.Member);
+useByConstValue(SS.Member);
+useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+SS.Member.setBool(true);
+  }
+  for (Struct SS : View>()) {
+SS.Member[1];
+  }
+  for (Struct SS : View>()) {
+mutate(SS.Member);
+  }
+  for (Struct SS : View>()) {
+mutate(&SS.Member);
+  }
+}
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -432,6 +432,14 @@
   `IgnoreTemplateInstantiations` option to optionally ignore virtual

[PATCH] D97567: [clang-tidy] performance-* checks: Also allow allow member expressions to be used in a const manner.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 543307.
xgupta added a comment.

Address comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97567/new/

https://reviews.llvm.org/D97567

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
  clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -114,8 +114,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -140,8 +140,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -172,8 +172,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -199,8 +199,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
@@ -843,3 +843,36 @@
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+auto Copy = Orig;
+Copy.Member.nonConstMethod();
+  }
+  {
+auto Copy = Orig;
+mutate(Copy.Member);
+  }
+  {
+auto Copy = Orig;
+mutate(&Copy.Member);
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,38 @@
 // SS : createView(*ValueReturningIterator())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+// CHECK-FIXES: for (const Struct& SS : View>()) {
+auto MemberCopy = SS.Member;
+const auto &ConstRef = SS.Member;
+bool b = SS.Member.constMethod();
+use(SS.Member);
+useByConstValue(SS.Member);
+useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+SS.Member.setBool(true);
+  }
+  for (Struct SS : View>()) {
+SS.Member[1];
+  }
+  for (Struct SS : View>()) {
+mutate(SS.Member);
+  }
+  for (Struct SS : View>()) {
+mutate(&SS.Member);
+  }
+}
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -432,6 +432,14 @@
   `IgnoreTemplateInstantiations` option to optionally ignore virtual function
   overrides that are part of template instantiations.
 
+- Improved :doc:`performance-for-range-copy
+  `
+  check

[PATCH] D97567: [clang-tidy] performance-* checks: Also allow allow member expressions to be used in a const manner.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 543300.
xgupta marked 2 inline comments as done.
xgupta added a comment.

minor update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97567/new/

https://reviews.llvm.org/D97567

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
  clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -114,8 +114,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -140,8 +140,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -172,8 +172,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -199,8 +199,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
@@ -843,3 +843,36 @@
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+auto Copy = Orig;
+Copy.Member.nonConstMethod();
+  }
+  {
+auto Copy = Orig;
+mutate(Copy.Member);
+  }
+  {
+auto Copy = Orig;
+mutate(&Copy.Member);
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,38 @@
 // SS : createView(*ValueReturningIterator())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+// CHECK-FIXES: for (const Struct& SS : View>()) {
+auto MemberCopy = SS.Member;
+const auto &ConstRef = SS.Member;
+bool b = SS.Member.constMethod();
+use(SS.Member);
+useByConstValue(SS.Member);
+useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+SS.Member.setBool(true);
+  }
+  for (Struct SS : View>()) {
+SS.Member[1];
+  }
+  for (Struct SS : View>()) {
+mutate(SS.Member);
+  }
+  for (Struct SS : View>()) {
+mutate(&SS.Member);
+  }
+}
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -520,6 +520,22 @@
   support unscoped enumerations through instances and fixed usage of anonymous
   structs or classes.
 
+- Improved :doc:`performance-for-range-copy
+  `
+

[PATCH] D97567: [clang-tidy] performance-* checks: Also allow allow member expressions to be used in a const manner.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 543299.
xgupta added a comment.

Update release note


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97567/new/

https://reviews.llvm.org/D97567

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
  clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -114,8 +114,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -140,8 +140,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -172,8 +172,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -199,8 +199,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
@@ -843,3 +843,36 @@
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+auto Copy = Orig;
+Copy.Member.nonConstMethod();
+  }
+  {
+auto Copy = Orig;
+mutate(Copy.Member);
+  }
+  {
+auto Copy = Orig;
+mutate(&Copy.Member);
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,37 @@
 // SS : createView(*ValueReturningIterator())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+// CHECK-FIXES: for (const Struct& SS : View>()) {
+auto MemberCopy = SS.Member;
+const auto &ConstRef = SS.Member;
+bool b = SS.Member.constMethod();
+use(SS.Member);
+useByConstValue(SS.Member);
+useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+SS.Member.setBool(true);
+  }
+  for (Struct SS : View>()) {
+SS.Member[1];
+  }
+  for (Struct SS : View>()) {
+mutate(SS.Member);
+  }
+  for (Struct SS : View>()) {
+mutate(&SS.Member);
+  }
+}
\ No newline at end of file
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -520,6 +520,22 @@
   support unscoped enumerations through instances and fixed usage of anonymous
   structs or classes.
 
+- Improved :doc:`performance-inefficient-vector-operation

[PATCH] D97567: [clang-tidy] performance-* checks: Also allow allow member expressions to be used in a const manner.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 543287.
xgupta added a comment.

Added release note and fix unit test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97567/new/

https://reviews.llvm.org/D97567

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
  clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -114,8 +114,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -140,8 +140,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -172,8 +172,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
@@ -199,8 +199,8 @@
   (void)*⌖
   S copy1 = /*const*/target;
   S copy2(/*const*/target);
-  useInt(target.int_member);
-  useIntConstRef(target.int_member);
+  useInt(/*const*/target.int_member);
+  useIntConstRef(/*const*/target.int_member);
   useIntPtr(target.ptr_member);
   useIntConstPtr(&target.int_member);
 }
Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
@@ -843,3 +843,36 @@
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+auto Copy = Orig;
+Copy.Member.nonConstMethod();
+  }
+  {
+auto Copy = Orig;
+mutate(Copy.Member);
+  }
+  {
+auto Copy = Orig;
+mutate(&Copy.Member);
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,37 @@
 // SS : createView(*ValueReturningIterator())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+// CHECK-FIXES: for (const Struct& SS : View>()) {
+auto MemberCopy = SS.Member;
+const auto &ConstRef = SS.Member;
+bool b = SS.Member.constMethod();
+use(SS.Member);
+useByConstValue(SS.Member);
+useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+SS.Member.setBool(true);
+  }
+  for (Struct SS : View>()) {
+SS.Member[1];
+  }
+  for (Struct SS : View>()) {
+mutate(SS.Member);
+  }
+  for (Struct SS : View>()) {
+mutate(&SS.Member);
+  }
+}
\ No newline at end of file
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -520,6 +520,17 @@
   support unscoped enumerations through instances and fixed usage of anonymous
   structs or classes.
 
+- Improved :doc:`performance-unneces

[PATCH] D78223: [clang-tidy] performance-range-for-copy only for copy.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.
Herald added subscribers: PiotrZSL, carlosgalvezp.
Herald added a project: All.

Reverse ping, this patch was never gets committed after being accepted.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78223/new/

https://reviews.llvm.org/D78223

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82784: [clang-tidy] For `run-clang-tidy.py` do not treat `allow_enabling_alpha_checkers` as a none value.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta closed this revision.
xgupta added a comment.
Herald added subscribers: wangpc, PiotrZSL, carlosgalvezp.
Herald added a reviewer: njames93.
Herald added projects: clang-tools-extra, All.

This was committed in 
https://github.com/llvm/llvm-project/commit/03ded5497a2f458b6af054fa7bac0da0240e7b7a.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82784/new/

https://reviews.llvm.org/D82784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97567: [clang-tidy] performance-* checks: Also allow allow member expressions to be used in a const manner.

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 543282.
xgupta added a comment.

Rebase and address one last comment.

I think now it will be fine to commit?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97567/new/

https://reviews.llvm.org/D97567

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
@@ -843,3 +843,36 @@
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+auto Copy = Orig;
+Copy.Member.nonConstMethod();
+  }
+  {
+auto Copy = Orig;
+mutate(Copy.Member);
+  }
+  {
+auto Copy = Orig;
+mutate(&Copy.Member);
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,37 @@
 // SS : createView(*ValueReturningIterator())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+// CHECK-FIXES: for (const Struct& SS : View>()) {
+auto MemberCopy = SS.Member;
+const auto &ConstRef = SS.Member;
+bool b = SS.Member.constMethod();
+use(SS.Member);
+useByConstValue(SS.Member);
+useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+Mutable Member;
+  };
+  for (Struct SS : View>()) {
+SS.Member.setBool(true);
+  }
+  for (Struct SS : View>()) {
+SS.Member[1];
+  }
+  for (Struct SS : View>()) {
+mutate(SS.Member);
+  }
+  for (Struct SS : View>()) {
+mutate(&SS.Member);
+  }
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -43,14 +43,18 @@
ASTContext &Context) {
   auto DeclRefToVar =
   declRefExpr(to(varDecl(equalsNode(&VarDecl.bind("declRef");
+  auto MemberExprOfVar = memberExpr(hasObjectExpression(DeclRefToVar));
+  auto DeclRefToVarOrMemberExprOfVar =
+  stmt(anyOf(DeclRefToVar, MemberExprOfVar));
   auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
   // Match method call expressions where the variable is referenced as the this
   // implicit object argument and operator call expression for member operators
   // where the variable is the 0-th argument.
   auto Matches = match(
-  findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
+  findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee,
+on(DeclRefToVarOrMemberExprOfVar)),
  cxxOperatorCallExpr(ConstMethodCallee,
- hasArgument(0, DeclRefToVar),
+ hasArgument(0, DeclRefToVarOrMemberExprOfVar),
   Stmt, Context);
   SmallPtrSet DeclRefs;
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
@@ -62,21 +66,22 @@
   ConstReferenceOrValue,
   substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
+  DeclRefToVarOrMemberExprOfVar,
+  parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   // References and pointers to const assignments.
   Matches =
   match(findAll(declStmt(
 has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
-hasInitializer(ignoringImpCasts(DeclRefToVar)),
+   

[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM, but we can wait for a couple of before committing in case other reviewers 
have more review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144748/new/

https://reviews.llvm.org/D144748

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101037: [clang-tidy] Change shebang from python to python3

[Shivam Gupta via Phabricator via cfe-commits]

xgupta closed this revision.
xgupta added a comment.
Herald added a reviewer: njames93.
Herald added a subscriber: PiotrZSL.
Herald added a project: All.

https://reviews.llvm.org/rG475440703238ca32adab6c3fe5e0039c3f96d1a5 committed 
the remaining shebang change from python to python3.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101037/new/

https://reviews.llvm.org/D101037

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Other than these three points everything looks good to me.




Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:71
+
+bool EmptyCatchCheck::isLanguageVersionSupported(
+const LangOptions &LangOpts) const {

This can be defined in the header file itself like other checks.



Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:76
+
+std::optional EmptyCatchCheck::getCheckTraversalKind() const {
+  return TK_IgnoreUnlessSpelledInSource;

This can be defined in the header file itself like other checks.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp:3
+// RUN: -config="{CheckOptions: [{key: 
bugprone-empty-catch.AllowEmptyCatchForExceptions, value: 
'::SafeException;WarnException'}, \
+// RUN:{key: bugprone-empty-catch.IgnoreCatchWithKeywords, value: 
'@IGNORE;@TODO'}]}"
+

If TODO is in default then it does not require to be in value here, right?
I tested without that, it gives the warning. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144748/new/

https://reviews.llvm.org/D144748

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Wanted to point out that there is a clang-tidy check in review doing the 
similar thing - https://reviews.llvm.org/D144429.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142800/new/

https://reviews.llvm.org/D142800

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

This LGTM, but can't officially accept the patch so please wait for another 
reviewer to accept it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Just normal ping, @aaron.ballman, can please take a look, pre-merge checks are 
also passing now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 542846.
xgupta added a comment.

update test for Windows system


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4511652 , @aaron.ballman 
wrote:

> In the x86 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is true, so 
> we test the other expression; because `Size` is greater than `__SIZE_MAX__` 
> the function returns false and the second static assertion fails as expected.
> In the x64 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is false 
> (first static assertion fails) so we shouldn't even be evaluating the RHS of 
> the `&&` to see if it's tautological because it can't contribute to the 
> expression result, right?

Yes, I agree with both statements but what is odd in current behavior?

It seems to me uint64_t is unsigned long in , not unsigned long long 
so the new test added in this patch (with `typedef unsigned long uint64_t`) is 
now passing.

Behavior when the patch is applied-

  $ ./bin/clang++ -Wall -fsyntax-only test.cpp -m64  -Wtype-limits 
  test.cpp:8:11: warning: result of comparison 'uint64_t' (aka 'unsigned long 
long') > 18446744073709551615 is always false 
[-Wtautological-type-limit-compare]
  8 |  Size > static_cast(__SIZE_MAX__)) // no-warning
|   ^ ~~~
  test.cpp:13:15: error: static assertion failed due to requirement 
'sizeof(unsigned long) < sizeof(unsigned long long)': 
 13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  test.cpp:13:35: note: expression evaluates to '8 < 8'
 13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  1 warning and 1 error generated.

  $ ./bin/clang++ -Wall -fsyntax-only test1.cpp -m64  -Wtype-limits 
  test1.cpp:12:15: error: static assertion failed due to requirement 
'sizeof(unsigned long) < sizeof(unsigned long)': 
 12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  test1.cpp:12:35: note: expression evaluates to '8 < 8'
 12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  1 error generated.

where test.cpp using wrapper and test1.cpp is using standard headers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 541866.
xgupta added a comment.

Add test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+
+typedef unsigned long uint64_t;
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+
+typedef unsigned long uint64_t;
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Thanks, @nickdesaulniers for reviewing and @nathanchance for testing the change.

@aaron.ballman, I also agree with @xbolva00 seems warning in kernel code is 
valid but I also agree with your comment about macro, may be better to track 
the macro-related issue with another GitHub issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdfdfd306cfaf: [Clang] Fix -Wconstant-logical-operand when 
LHS is a constant (authored by shivam-amd).

Changed prior to commit:
  https://reviews.llvm.org/D142609?vs=540922&id=541850#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,6 +44,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence th

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D142609#4510995 , @aaron.ballman 
wrote:

> In D142609#4510596 , @xbolva00 
> wrote:
>
>> In my opinion it makes sense to adjust that kernel code based on this 
>> warning in the current inplementation state.
>>
>> @aaron.ballman ?
>
> I think that use of macros for any of the constant values in the expression 
> should silence the diagnostic on the assumption that the macro value changes 
> with different configurations and so it's only constant in one configuration 
> mode. Alternatively, I could see an argument to use a different diagnostic 
> group when macros are involved so that users can silence macro-related 
> constant warnings while not losing non-macro cases. WDYT?

IIUC in this patch I am making more like an NFC change and this could be a 
separate issue. WDYT @nickdesaulniers? Please review it so we can commit it 
soon.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4511392 , @aaron.ballman 
wrote:

> Whether `size_t` comes from the system header or whether it's manually 
> deduced from `decltype(sizeof(0))` should make no difference as far as the 
> frontend is concerned; they should resolve to the same type. Can you explain 
> the test failure you're seeing in a bit more detail?



  : 'RUN: at line 1';   /home/shivam/.llvm/llvm-project/build/bin/clang -cc1 
-internal-isystem /home/shivam/.llvm/llvm-project/build/lib/clang/17/include 
-nostdsysteminc 
/home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp 
-fsyntax-only -Wtautological-type-limit-compare -verify
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  error: 'warning' diagnostics seen but not expected: 
File /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp 
Line 12: result of comparison 'uint64_t' (aka 'unsigned long long') > 
18446744073709551615 is always false
  1 error generated.

I think the issue is with __SIZE_MAX__ vs 
std::numeric_limits::max() but not sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D142609#4507696 , @nathanchance 
wrote:

> I took the most recent version for a spin against the Linux kernel. The 
> couple of issues that a previous revision of the warning flagged 
> (https://github.com/ClangBuiltLinux/linux/issues/1806, 
> https://github.com/ClangBuiltLinux/linux/issues/1807) are no longer visible 
> (that seems intentional because both of those came from macro expressions) 
> but I see a new one along a similar line as those:
>
> https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343
>
>   In file included from drivers/gpu/drm/v3d/v3d_bo.c:25:
>   drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with 
> constant operand [-Wconstant-logical-operand]
> 343 | if (NSEC_PER_SEC % HZ &&
> | ~ ^
>   drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation
> 343 | if (NSEC_PER_SEC % HZ &&
> |   ^~
> |   &
>   drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this 
> warning
>   1 warning generated.
>
> Another minimized example showing how the warning can trigger with different 
> configurations:
>
>   $ cat x.c
>   #define A 1000
>   #define B 300
>   #define C 250
>   #define D 100
>   
>   int bar(void);
>   int baz(void);
>   
>   int foo(void)
>   {
>   if (A % B && bar()) // 1000 % 300 = 100, warning
>   return -3;
>   
>   if (A % C && bar()) // 1000 % 250 = 0, no warning
>   return -2;
>   
>   if (A % D && bar()) // 1000 % 100 = 0, no warning
>   return -1;
>   
>   return baz();
>   }
>   
>   $ clang -Wall -fsyntax-only x.c
>   x.c:11:12: warning: use of logical '&&' with constant operand 
> [-Wconstant-logical-operand]
>  11 | if (A % B && bar())
> | ~ ^
>   x.c:11:12: note: use '&' for a bitwise operation
>  11 | if (A % B && bar())
> |   ^~
> |   &
>   x.c:11:12: note: remove constant to silence this warning
>   1 warning generated.
>
> I am sure we can send a patch making that a boolean expression (although it 
> is duplicated in a few places it seems so multiple patches will be needed) 
> but I figured I would report it just in case there was something that should 
> be changed with the warning, since I see there was already some discussion 
> around not warning for macros and this seems along a similar line.

WDYT @xbolva00,  It is a valid warning or more modification is required in the 
patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4507124 , @aaron.ballman 
wrote:

> In D155457#4507107 , @xgupta wrote:
>
>> In D155457#4506405 , @xbolva00 
>> wrote:
>>
>>> You should add a testcase which uses “expected no diagnostics”.
>>
>> There is some issue writing test
>>
>>   $ cat type-limit-compare.cpp
>>   // RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare 
>> -verify %s
>>   // expected-no-diagnostics
>>   
>>   #include 
>>   #include 
>>   #include 
>>   
>>   bool foo(uint64_t Size) {
>> if (sizeof(std::size_t) < sizeof(uint64_t) &&
>>Size > 
>> static_cast(std::numeric_limits::max())) // no-warning
>>   return false;
>> return true;
>>   }
>>
>> failed with
>>
>>   $ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 
>> 'cstddef' file not found
>>   4 | #include 
>> |  ^
>
> We typically do not include any system headers (STL or otherwise) as part of 
> the compiler tests; that would test whatever is found on the test system 
> instead of a consistent test. Instead, I'd recommend doing:
>
>   namespace std {
>   using size_t = decltype(sizeof(0));
>   }
>
> Similarly, you can replace `uint64_t` with `unsigned long long` and the 
> `numeric_limits` call with `__SIZE_MAX__`

I see, Thanks,  but there is another thing, writing this way compiler emits a 
warning as the check to exclude the warning is based on `size_t` so the test 
case is not passed.

  // RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
  
  // expected-no-diagnostics
  
  typedef unsigned long long uint64_t;
  namespace std {
  using size_t = decltype(sizeof(0));
  } // namespace std
  
  bool func(uint64_t Size) {
if (sizeof(std::size_t) < sizeof(uint64_t) &&
   Size > (uint64_t)(__SIZE_MAX__))
  return false;
return true;
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4506405 , @xbolva00 wrote:

> You should add a testcase which uses “expected no diagnostics”.

There is some issue writing test

  $ cat type-limit-compare.cpp
  // RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify %s
  // expected-no-diagnostics
  
  #include 
  #include 
  #include 
  
  bool foo(uint64_t Size) {
if (sizeof(std::size_t) < sizeof(uint64_t) &&
   Size > static_cast(std::numeric_limits::max())) 
// no-warning
  return false;
return true;
  }

failed with

  $ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 
'cstddef' file not found
  4 | #include 
|  ^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

I am not sure if this is correct but this passes `ninja check-clang`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155457/new/

https://reviews.llvm.org/D155457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta created this revision.
xgupta added reviewers: aaron.ballman, xbolva00.
Herald added a project: All.
xgupta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The issue with size_t comes when we are trying to addd
-Wtype-limits to -Wextra for GCC compatibility in review 
https://reviews.llvm.org/D142826.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13363,6 +13363,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13363,6 +13363,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540922.
xgupta added a comment.

Address xbolva00's comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -43,6 +43,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
@@ -65,6 +68,8 @@
// expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
// 

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13635
+  if (ECDHS && ECDHS->getInitVal() != 0 && ECDHS->getInitVal() != 1)
+EnumConstantInBoolContext = true;
+}

xgupta wrote:
> nickdesaulniers wrote:
> > Hmm...I wonder if we now care about which `ExprResult` contained an enum 
> > constant in bool context?
> IIUC you mean EnumConstantInBoolContext is not required, but then test cases 
> start failing if I will not use it in diagnoseLogicalInsteadOfBitwise above.
> Like this - 
> 
> $ build/bin/llvm-lit -v clang/test/Sema
> 
> Command Output (stderr):
> --
> error: 'warning' diagnostics seen but not expected: 
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 52: use of logical '||' with constant operand
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 56: use of logical '||' with constant operand
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 68: use of logical '&&' with constant operand
> error: 'note' diagnostics seen but not expected: 
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 52: use '|' for a bitwise operation
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 56: use '|' for a bitwise operation
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 68: use '&' for a bitwise operation
>   File 
> /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
> Line 68: remove constant to silence this warning
> 7 errors generated.
> 
> --
> Failed Tests (1):
>   Clang :: Sema/warn-int-in-bool-context.c
> 
> 
> Testing Time: 0.04s
>   Failed: 1
> 
I will mark this as done, EnumConstantInBoolContext is required for 
warn_enum_constant_in_bool_context warning in the below code.



Comment at: clang/lib/Sema/SemaExpr.cpp:13588
+   bool EnumConstantInBoolContext) {
+  if (!EnumConstantInBoolContext && Op1.get()->getType()->isIntegerType() &&
+  !Op1.get()->getType()->isBooleanType() &&

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > This is the only use of `EnumConstantInBoolContext` in 
> > `Sema::diagnoseLogicalInsteadOfBitwise`. It's being used to elide the 
> > entirety of the method.  In that case, it seems wasteful to bother to pass 
> > it as a parameter.  Instead, I don't think 
> > `Sema::diagnoseLogicalInsteadOfBitwise` should be called if 
> > `EnumConstantInBoolContext` is `true`.
> > 
> > If you remove the parameter `EnumConstantInBoolContext` then...
> Do you mind pulling the types into dedicated variables, then reusing them? I 
> kind of hate seeing verbose OpX.get()->getType() so much in this method.
> 
> Type T1 = Op1.get()->getType();
> Type T2 = Op2.get()->getType();
> 
> if (T1->isIntegerType() && !T1->isBooleanType() ...
>   ...
Done by the first comment- 
"Every reference to Op1 and Op2 immediately calls .get() on it. That's 
annoying. How about Sema::diagnoseLogicalInsteadOfBitwise accepts Expr* (or 
whatever ExprResult::get returns), and we call .get() in the caller?"





Comment at: clang/test/SemaCXX/expressions.cpp:146-148
   #define Y2 2
   bool r2 = X || Y2; // expected-warning {{use of logical '||' with constant 
operand}} \
  // expected-note {{use '|' for a bitwise operation}}

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > So I think we'll want to change this test.
> > 
> > See commit d6eb2b9f4d4fc236376e3a5a7b8faa31e8dd427d that introduced it.
> > 
> > If we have a constant that was defined via macro, we DONT want to warn for 
> > it.
> Another related issue is that sometimes we set these constants via `-D` 
> flags. I wonder if that's a clang bug that those aren't considered as having 
> a valid macro id?
> 
> See also https://github.com/ClangBuiltLinux/linux/issues/1806
> So I think we'll want to change this test.
But it is not failing, what changes you want there?

> I wonder if that's a clang bug that those aren't considered as having a valid 
> macro id?
 Will it require to address in this patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540914.
xgupta marked 5 inline comments as done.
xgupta added a comment.

Address comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -43,6 +43,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
@@ -65,6 +68,8 @@
// expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant opera

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D4784#4504690 , @PiotrZSL wrote:

> Just few nits (column numbers in test, missing doxygen comment, ...).
> Please fix those before committing.
>
> Except that, looking good to me.

Thanks for this nice code review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG42179bbf6bcc: [clang-tidy] Add check for possibly incomplete 
switch statements (authored by shivam-amd).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case %t -- -- -fno-delayed-template-parsing
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+
+  int getValue(void);
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  int getValue(void);
+  switch (getValue()) { // no-warning
+  case 0:
+break;
+default:
+break;
+  }
+}
+
+template
+void testTemplate(T Value) {
+  switch (Value) {
+case 0:
+  break;
+  }
+}
+
+void exampleUsage() {
+  testTemplate(5);
+  testTemplate(EnumType::eE2);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_,
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540857.
xgupta marked 6 inline comments as done.
xgupta added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case %t -- -- -fno-delayed-template-parsing
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+
+  int getValue(void);
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  int getValue(void);
+  switch (getValue()) { // no-warning
+  case 0:
+break;
+default:
+break;
+  }
+}
+
+template
+void testTemplate(T Value) {
+  switch (Value) {
+case 0:
+  break;
+  }
+}
+
+void exampleUsage() {
+  testTemplate(5);
+  testTemplate(EnumType::eE2);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_,
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings (comes under -Wswitch)
+   when a switch statement does not handle all enum values. This check f

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540845.
xgupta marked an inline comment as done.
xgupta added a comment.

Updated testcase [WIP]


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case %t -- -- -fno-delayed-template-parsing
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+
+  int getValue(void);
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  int getValue(void);
+  switch (getValue()) { // no-warning
+  case 0:
+break;
+default:
+break;
+  }
+}
+
+template
+void testTemplate(T Value) {
+  switch (Value) {
+case 0:
+  break;
+  }
+}
+
+void exampleUsage() {
+  testTemplate(5);
+  testTemplate(EnumType::eE2);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_,
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings (comes under -Wswitch)
+   when a switch statement does not handle all enum values. Thi

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:19
+  Finder->addMatcher(
+  switchStmt(has(implicitCastExpr().bind("cast")),
+ unless(hasAncestor(switchStmt(has(defaultStmt())

PiotrZSL wrote:
> xgupta wrote:
> > PiotrZSL wrote:
> > > this should be something like:
> > > ```hasCondition(expr(hasType(qualType(hasCanonicalType(unless(hasDeclaration(enumDecl()))```
> > > Or you can verify just if type is integral type.
> > > Note that with modern C++ you may have init statements in enums.
> > > 
> > For some reason, the check is giving warning for enum cases and I couldn't 
> > understand why, can you please help?
> Add getCheckTraversalKind and check again
Wow, feel like a magic!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540818.
xgupta marked 5 inline comments as done.
xgupta added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,59 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+template
+void testTemplate(T Value)
+{
+// CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+switch (Value) {
+case 0:
+break;
+}
+}
+
+void positive() {
+int I1 = 0;
+testTemplate(I1);
+
+MyInt I2 = 0;
+testTemplate(I2);
+
+int getValue();
+testTemplate(getValue());
+}
+
+void negative() {
+enum E { eE1 };
+E E1 = eE1;
+testTemplate(E1);
+
+MyEnum E2 = eE2;
+testTemplate(E2);
+
+int I1 = 0;
+switch (I1) {
+case 0:
+break;
+default:
+break;
+}
+
+MyInt I2 = 0;
+switch (I2) {
+case 0:
+break;
+default:
+break;
+}
+
+int getValue();
+switch (getValue()) {
+case 0:
+break;
+default:
+break;
+}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_,
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings (comes under -Wswitch)
+   when a switch statement does not handle all enum values. This check focuses
+   on non-enum types where the compiler warnings may not be present.
+
+.. seealso::
+   The `CppCoreGuideline ES.79 `_
+   provide guidelines on switch statements, including the recommendation to
+   always provide a default case.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-ex

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta marked 3 inline comments as done.
xgupta added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:16-29
+void SwitchMissingDefaultCaseCheck::registerMatchers(
+ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(has(implicitCastExpr().bind("cast")),
+ unless(hasAncestor(switchStmt(has(defaultStmt())
+  .bind("switch"),
+  this);

PiotrZSL wrote:
> I do not like that implicitCastExpr.
> 
> this is AST for your example:
> ```
>`-SwitchStmt 
>   |-ImplicitCastExpr  'int' 
>   | `-DeclRefExpr  'int' lvalue Var 0x555de93bc200 'i' 'int'
>   `-CompoundStmt 
> `-CaseStmt 
>   |-ConstantExpr  'int'
>   | |-value: Int 0
>   | `-IntegerLiteral  'int' 0
>   `-BreakStmt 
> ```
> 
> look that case there is only because we got cast from rvalue to lvalue.
> if you write example like this:
> 
> ```
> int getValue();
> 
> void Positive() {
>   switch (getValue()) {
>   case 0:
> break;
>   }
> }
> 
> ```
> 
> there is not cast because AST will look like this:
> ```
> -SwitchStmt 
>   |-CallExpr  'int'
>   | `-ImplicitCastExpr  'int (*)()' 
>   |   `-DeclRefExpr  'int ()' lvalue Function 0x5573a3b38100 
> 'getValue' 'int ()'
>   `-CompoundStmt 
> `-CaseStmt 
>   |-ConstantExpr  'int'
>   | |-value: Int 0
>   | `-IntegerLiteral  'int' 0
>   `-BreakStmt 
> ```
> 
> So add test with rvalue int... remove that implicit cast checks, and use 
> IgnoreUnlessSpelledInSource (it will remove IntegralCast cast from enum to 
> int, so hasCondition would match enum directly)
Removed the implicit cast but couldn't get how to use 
IgnoreUnlessSpelledInSource here.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:19
+  Finder->addMatcher(
+  switchStmt(has(implicitCastExpr().bind("cast")),
+ unless(hasAncestor(switchStmt(has(defaultStmt())

PiotrZSL wrote:
> this should be something like:
> ```hasCondition(expr(hasType(qualType(hasCanonicalType(unless(hasDeclaration(enumDecl()))```
> Or you can verify just if type is integral type.
> Note that with modern C++ you may have init statements in enums.
> 
For some reason, the check is giving warning for enum cases and I couldn't 
understand why, can you please help?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540798.
xgupta marked an inline comment as done.
xgupta added a comment.

Address comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,67 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+
+  int getValue();
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  int getValue();
+  switch (getValue()) { // no-warning
+  case 0:
+break;
+default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings (comes under -Wswitch)
+   when a switch statement does not handle all enum values. This check focuses
+   on non-enum types where the compiler warnings may not be present.
+
+.. seealso::
+   The `CppCoreGuideline ES.79 `_
+   prov

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta marked an inline comment as done.
xgupta added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:52
+.. note::
+   Enum types are already covered by compiler warnings when a switch statement
+   does not handle all enum values. This check focuses on non-enum types where

PiotrZSL wrote:
> xgupta wrote:
> > PiotrZSL wrote:
> > > would be nice to list them
> > Didn't understand, what should I list, non-enum types?
> No, not a types, compiler warnings (ones for enums). Simply so when user 
> enable this check for integers, He/she could make sure that enum specific 
> warnings are also enabled.
> and remove this list that you added, as you cannot do switch on pointers or 
> floating point types.
I can see there is -Wswitch which I have mentioned.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540119.
xgupta marked an inline comment as done.
xgupta added a comment.

Address latest two comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,67 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+
+  int getValue();
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  int getValue();
+  switch (getValue()) { // no-warning
+  case 0:
+break;
+default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings (comes under -Wswitch)
+   when a switch statement does not handle all enum values. This check focuses
+   on non-enum types where the compiler warnings may not be present.
+
+.. seealso::
+   The `CppCoreGuideline ES.79 

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 540102.
xgupta added a comment.

Updated SwitchMissingDefaultCaseCheck.cpp but still WIP
Will look again on saturday/sunday


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,66 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+typedef int MyInt;
+typedef enum { eE2 } MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+
+  int getValue();
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  int getValue();
+  switch (getValue()) { // no-warning
+  case 0:
+break;
+default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings when a switch statement
+   does not handle all enum values. This check focuses on non-enum types where
+   the compiler warnings may not be present. Some common examples of non-enum
+   types include:
+   - Integral types (int, char, short, etc.)
+   - Floating-point types (float, double)
+

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:225
+
+  Ensures that incomplete switch statements without default cases are
+  flagged, covering cases beyond enums where the compiler may not issue 
warnings.

PiotrZSL wrote:
> actually that are `complete` switch statements, developer covered everything 
> He/She wanted, so other wording should be used
Sure.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:226
+  Ensures that incomplete switch statements without default cases are
+  flagged, covering cases beyond enums where the compiler may not issue 
warnings.
+

PiotrZSL wrote:
> this suggest that enums are covered by check, and thats false
Corrected.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:46-49
+Options
+---
+
+This check does not have any configurable options.

PiotrZSL wrote:
> this isnt needed, no options, not point to mention them
Sure, will remove.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:52
+.. note::
+   Enum types are already covered by compiler warnings when a switch statement
+   does not handle all enum values. This check focuses on non-enum types where

PiotrZSL wrote:
> would be nice to list them
Didn't understand, what should I list, non-enum types?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:58
+   The C++ Core Guidelines provide guidelines on switch statements, including
+   the recommendation to always provide a default case: :cpp:dir:`C.89`.

PiotrZSL wrote:
> C.89 is C.89: Make a hash noexcept
> There is "ES.79: Use default to handle common cases (only)" but thats only 
> about enums., still could be put here as reference, in such case you should 
> put link
Accurate, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539857.
xgupta marked 12 inline comments as done.
xgupta added a comment.

Address all comments excpet in SwitchMissingDefaultCaseCheck.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,51 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+typedef int MyInt;
+typedef enum { eE2 } MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings when a switch statement
+   does not handle all enum values. This check focuses on non-enum types where
+   the compiler warnings may not be present. Some common examples of non-enum
+   types include:
+   - Integral types (int, char, short, etc.)
+   - Floating-point types (float, double)
+   - Pointer types
+   - User-defined types without enum properties
+
+.. seealso::
+   The `CppCoreGuideline ES.79 `_
+   provide guidelines on switch statements, including the recommendation to
+   always provide a default case.
Index: clang-tools-extra/d

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539602.
xgupta added a comment.

Add documentation


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+void Positive() {
+  int i = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst
@@ -0,0 +1,58 @@
+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+===
+
+Detects incomplete switch statements without a default case.
+
+For exmaple:
+
+.. code-block:: c++
+
+  // Example 1:
+  switch (i) { // warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  case 0:
+break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+
+Incomplete switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Options
+---
+
+This check does not have any configurable options.
+
+.. note::
+   Enum types are already covered by compiler warnings when a switch statement
+   does not handle all enum values. This check focuses on non-enum types where
+   the compiler warnings may not be present.
+
+.. seealso::
+   The C++ Core Guidelines provide guidelines on switch statements, including
+   the recommendation to always provide a default case: :cpp:dir:`C.89`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,6 +219,12 @@
   Enforces consistent token representation for invoked binary, unary and
   overloaded operators in C++ code.
 
+- New :doc:`bugprone-switch-missing-default-case
+  ` check.
+
+  Ensures that incomplete switch statements without default cases are
+  flagged, covering cases beyond enums where the compiler may not issue warnings.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539593.
xgupta marked 3 inline comments as done.
xgupta added a comment.

Address more comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case -- %t
+
+void Positive() {
+  int i = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,6 +219,12 @@
   Enforces consistent token representation for invoked binary, unary and
   overloaded operators in C++ code.
 
+- New :doc:`bugprone-switch-missing-default-case
+  ` check.
+
+  Ensures that incomplete switch statements without default cases are
+  flagged, covering cases beyond enums where the compiler may not issue warnings.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
@@ -0,0 +1,29 @@
+//===--- SwitchMissingDefaultCaseCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::tidy::bugprone {
+
+class SwitchMissingDefaultCaseCheck : public ClangTidyCheck {
+public:
+  SwitchMissingDefaultCaseCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
@@ -0,0 +1,36 @@
+//===--- SwitchMissingDefaultCaseCheck.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "SwitchMissingDefaultCaseCheck.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void SwitchMissingDefaultCaseCheck::registerMatchers(
+ast_matchers::MatchFinder

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539558.
xgupta edited the summary of this revision.
xgupta added a comment.

Address some comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck -implicit-check-not="{{warning|error}}:" %s
+
+void Positive() {
+  int i = 0;
+  // CHECK: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [misc-incomplete-switch]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload `_,
`bugprone-implicit-widening-of-multiplication-result `_, "Yes"
`bugprone-inaccurate-erase `_, "Yes"
+   `bugprone-switch-missing-default-case `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
`bugprone-integer-division `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,6 +219,11 @@
   Enforces consistent token representation for invoked binary, unary and
   overloaded operators in C++ code.
 
+- New :doc:`bugprone-switch-missing-default-case
+  ` check.
+
+  Detects incomplete switch statements without a default case.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h
@@ -0,0 +1,29 @@
+//===--- SwitchMissingDefaultCaseCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::tidy::bugprone {
+
+class SwitchMissingDefaultCaseCheck : public ClangTidyCheck {
+public:
+  SwitchMissingDefaultCaseCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp
@@ -0,0 +1,36 @@
+//===--- SwitchMissingDefaultCaseCheck.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "SwitchMissingDefaultCaseCheck.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void SwitchMissingDefaultCaseCheck::registerMatchers(
+ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(has(implici

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539390.
xgupta added a comment.

Update ReleaseNotes.rst


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck -implicit-check-not="{{warning|error}}:" %s
+
+void Positive() {
+  int i = 0;
+  // CHECK: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [misc-incomplete-switch]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,6 +219,11 @@
   Enforces consistent token representation for invoked binary, unary and
   overloaded operators in C++ code.
 
+- New :doc:`misc-incomplete-switch
+  ` check.
+
+  Detects incomplete switch statements without a default case.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "DefinitionsInHeadersCheck.h"
 #include "HeaderIncludeCycleCheck.h"
 #include "IncludeCleanerCheck.h"
+#include "IncompleteSwitchCheck.h"
 #include "MisleadingBidirectional.h"
 #include "MisleadingIdentifier.h"
 #include "MisplacedConstCheck.h"
@@ -46,6 +47,8 @@
 CheckFactories.registerCheck(
 "misc-header-include-cycle");
 CheckFactories.registerCheck("misc-include-cleaner");
+CheckFactories.registerCheck(
+"misc-incomplete-switch");
 CheckFactories.registerCheck(
 "misc-misleading-bidirectional");
 CheckFactories.registerCheck(
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
@@ -0,0 +1,29 @@
+//===--- IncompleteSwitchCheck.h - clang-tidy *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::tidy::misc {
+
+class IncompleteSwitchCheck : public ClangTidyCheck {
+public:
+  IncompleteSwitchCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
@@ -0,0 +1,36 @@
+//===--- IncompleteSwitchCheck.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "IncompleteSwitchCheck.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+void IncompleteSwitchCheck::registerMatchers(
+ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(has(implicitCastExpr().bind("cast")),
+ unless(hasAncestor(swi

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539389.
xgupta added a comment.

Address bkramer's comments except one for "checking that you're casting from an 
enum".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck -implicit-check-not="{{warning|error}}:" %s
+
+void Positive() {
+  int i = 0;
+  // CHECK: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [misc-incomplete-switch]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "DefinitionsInHeadersCheck.h"
 #include "HeaderIncludeCycleCheck.h"
 #include "IncludeCleanerCheck.h"
+#include "IncompleteSwitchCheck.h"
 #include "MisleadingBidirectional.h"
 #include "MisleadingIdentifier.h"
 #include "MisplacedConstCheck.h"
@@ -46,6 +47,8 @@
 CheckFactories.registerCheck(
 "misc-header-include-cycle");
 CheckFactories.registerCheck("misc-include-cleaner");
+CheckFactories.registerCheck(
+"misc-incomplete-switch");
 CheckFactories.registerCheck(
 "misc-misleading-bidirectional");
 CheckFactories.registerCheck(
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
@@ -0,0 +1,29 @@
+//===--- IncompleteSwitchCheck.h - clang-tidy *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::tidy::misc {
+
+class IncompleteSwitchCheck : public ClangTidyCheck {
+public:
+  IncompleteSwitchCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
@@ -0,0 +1,36 @@
+//===--- IncompleteSwitchCheck.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "IncompleteSwitchCheck.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+void IncompleteSwitchCheck::registerMatchers(
+ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(has(implicitCastExpr().bind("cast")),
+ unless(hasAncestor(switchStmt(has(defaultStmt())
+  .bind("switch"),
+  this);
+}
+
+void IncompleteSwitchCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *c = Result.Nodes.getNodeAs("cast");
+  if (c->getCastKind() == CK_IntegralCast)
+return;
+
+  const auto *s = Result.Nodes.getNodeAs("switch");
+  diag(s->getSwitchLoc(), "switching on non-enum value without "
+  "default case may not cover all cases");

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539383.
xgupta added a comment.

.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck %s
+
+void Positive() {
+  int i = 0;
+  // CHECK: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [misc-incomplete-switch]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "DefinitionsInHeadersCheck.h"
 #include "HeaderIncludeCycleCheck.h"
 #include "IncludeCleanerCheck.h"
+#include "IncompleteSwitchCheck.h"
 #include "MisleadingBidirectional.h"
 #include "MisleadingIdentifier.h"
 #include "MisplacedConstCheck.h"
@@ -46,6 +47,7 @@
 CheckFactories.registerCheck(
 "misc-header-include-cycle");
 CheckFactories.registerCheck("misc-include-cleaner");
+CheckFactories.registerCheck("misc-incomplete-switch");
 CheckFactories.registerCheck(
 "misc-misleading-bidirectional");
 CheckFactories.registerCheck(
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
@@ -0,0 +1,31 @@
+//===--- IncompleteSwitchCheck.h - clang-tidy *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+
+class IncompleteSwitchCheck : public ClangTidyCheck {
+public:
+  IncompleteSwitchCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
@@ -0,0 +1,35 @@
+//===--- IncompleteSwitchCheck.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "IncompleteSwitchCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void IncompleteSwitchCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(hasDescendant(implicitCastExpr().bind("cast")),
+ unless(hasDescendant(defaultStmt(.bind("switch"),
+  this);
+}
+
+void IncompleteSwitchCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *c = Result.Nodes.getNodeAs("cast");
+  if (c->getCastKind() == CK_IntegralCast)
+return;
+
+  const auto *s = Result.Nodes.getNodeAs("switch");
+  diag(s->getSwitchLoc(), "switching on non-enum value without "
+  "default case may not cover all cases");
+}
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/misc/CMakeLists.txt
===

[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 539378.
xgupta retitled this revision from "[clang-tidy] Add check for possibly 
incomplete switch statements
" to "[clang-tidy] Add check for possibly incomplete switch statements".
xgupta added a reviewer: PiotrZSL.
xgupta added a comment.
Herald added a reviewer: njames93.
Herald added a project: clang-tools-extra.

Just updated the patch to trunk


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
  clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck %s
+
+void Positive() {
+  int i = 0;
+  // CHECK: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [misc-incomplete-switch]
+  switch (i) {
+  case 0:
+break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+break;
+  default:
+break;
+  }
+}
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "DefinitionsInHeadersCheck.h"
 #include "HeaderIncludeCycleCheck.h"
 #include "IncludeCleanerCheck.h"
+#include "IncompleteSwitchCheck.h"
 #include "MisleadingBidirectional.h"
 #include "MisleadingIdentifier.h"
 #include "MisplacedConstCheck.h"
@@ -46,6 +47,7 @@
 CheckFactories.registerCheck(
 "misc-header-include-cycle");
 CheckFactories.registerCheck("misc-include-cleaner");
+CheckFactories.registerCheck("misc-incomplete-switch");
 CheckFactories.registerCheck(
 "misc-misleading-bidirectional");
 CheckFactories.registerCheck(
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.h
@@ -0,0 +1,23 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+
+class IncompleteSwitchCheck : public ClangTidyCheck {
+public:
+  IncompleteSwitchCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
Index: clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp
@@ -0,0 +1,27 @@
+#include "IncompleteSwitchCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void IncompleteSwitchCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(hasDescendant(implicitCastExpr().bind("cast")),
+ unless(hasDescendant(defaultStmt(.bind("switch"),
+  this);
+}
+
+void IncompleteSwitchCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *c = Result.Nodes.getNodeAs("cast");
+  if (c->getCastKind() == CK_IntegralCast)
+return;
+
+  const auto *s = Result.Nodes.getNodeAs("switch");
+  diag(s->getSwitchLoc(), "switching on non-enum value without "
+  "default case may not cover all cases");
+}
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/misc/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -22,6 +22,7 @@
   ConfusableIdentifierCheck.cpp
   HeaderIncludeCycleCheck.cpp
   IncludeCleanerCheck.cpp
+  IncompleteSwitchCheck.cpp
   MiscTidyModule.cpp
   MisleadingBidirectional.cpp
   MisleadingIdentifier.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.l

[PATCH] D146370: [Clang][OpenMP]Solved the the always truth condition in Arm64

[Shivam Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc1ab19850d5c: [Clang][OpenMP]Solved the the always truth 
condition in Arm64 (authored by samuelmaina, committed by xgupta).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146370/new/

https://reviews.llvm.org/D146370

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -11160,7 +11160,7 @@
   if (Kind == ParamKindTy::Uniform)
 return false;
 
-  if (Kind == ParamKindTy::LinearUVal || ParamKindTy::LinearRef)
+  if (Kind == ParamKindTy::LinearUVal || Kind == ParamKindTy::LinearRef)
 return false;
 
   if ((Kind == ParamKindTy::Linear || Kind == ParamKindTy::LinearVal) &&


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -11160,7 +11160,7 @@
   if (Kind == ParamKindTy::Uniform)
 return false;
 
-  if (Kind == ParamKindTy::LinearUVal || ParamKindTy::LinearRef)
+  if (Kind == ParamKindTy::LinearUVal || Kind == ParamKindTy::LinearRef)
 return false;
 
   if ((Kind == ParamKindTy::Linear || Kind == ParamKindTy::LinearVal) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149713: [Sema] Avoid emitting warnings for constant destruction.

[Shivam Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd6a58babc85: [Sema] Avoid emitting warnings for constant 
destruction. (authored by pkasting, committed by xgupta).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149713/new/

https://reviews.llvm.org/D149713

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-exit-time-destructors.cpp
  clang/test/SemaCXX/warn-global-constructors.cpp


Index: clang/test/SemaCXX/warn-global-constructors.cpp
===
--- clang/test/SemaCXX/warn-global-constructors.cpp
+++ clang/test/SemaCXX/warn-global-constructors.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wglobal-constructors %s -verify
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wglobal-constructors %s 
-verify=expected,cxx11
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wglobal-constructors %s 
-verify=expected
 
 int opaque_int();
 
@@ -145,3 +146,22 @@
   const HasUnnamedBitfield nonConstexprConst{1}; // expected-warning {{global 
constructor}}
   HasUnnamedBitfield nonConstexprMutable{1}; // expected-warning {{global 
constructor}}
 }
+
+namespace test7 {
+#if __cplusplus >= 202002L
+#define CPP20_CONSTEXPR constexpr
+#else
+#define CPP20_CONSTEXPR
+#endif
+  struct S {
+CPP20_CONSTEXPR ~S() {}
+  };
+  S s; // cxx11-warning {{global destructor}}
+
+  struct T {
+CPP20_CONSTEXPR ~T() { if (b) {} }
+bool b;
+  };
+  T t; // expected-warning {{global destructor}}
+#undef CPP20_CONSTEXPR
+}
Index: clang/test/SemaCXX/warn-exit-time-destructors.cpp
===
--- clang/test/SemaCXX/warn-exit-time-destructors.cpp
+++ clang/test/SemaCXX/warn-exit-time-destructors.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wexit-time-destructors %s -verify
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wexit-time-destructors %s 
-verify=expected,cxx11
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wexit-time-destructors %s 
-verify=expected
 
 namespace test1 {
   struct A { ~A(); };
@@ -48,3 +49,22 @@
 struct A { ~A(); };
 [[clang::no_destroy]] A a; // no warning
 }
+
+namespace test5 {
+#if __cplusplus >= 202002L
+#define CPP20_CONSTEXPR constexpr
+#else
+#define CPP20_CONSTEXPR
+#endif
+  struct S {
+CPP20_CONSTEXPR ~S() {}
+  };
+  S s; // cxx11-warning {{exit-time destructor}}
+
+  struct T {
+CPP20_CONSTEXPR ~T() { if (b) {} }
+bool b;
+  };
+  T t; // expected-warning {{exit-time destructor}}
+#undef CPP20_CONSTEXPR
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15732,7 +15732,8 @@
 }
   }
 
-  if (!VD->hasGlobalStorage()) return;
+  if (!VD->hasGlobalStorage() || !VD->needsDestruction(Context))
+return;
 
   // Emit warning for non-trivial dtor in global scope (a real global,
   // class-static, function-static).


Index: clang/test/SemaCXX/warn-global-constructors.cpp
===
--- clang/test/SemaCXX/warn-global-constructors.cpp
+++ clang/test/SemaCXX/warn-global-constructors.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wglobal-constructors %s -verify
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wglobal-constructors %s -verify=expected,cxx11
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wglobal-constructors %s -verify=expected
 
 int opaque_int();
 
@@ -145,3 +146,22 @@
   const HasUnnamedBitfield nonConstexprConst{1}; // expected-warning {{global constructor}}
   HasUnnamedBitfield nonConstexprMutable{1}; // expected-warning {{global constructor}}
 }
+
+namespace test7 {
+#if __cplusplus >= 202002L
+#define CPP20_CONSTEXPR constexpr
+#else
+#define CPP20_CONSTEXPR
+#endif
+  struct S {
+CPP20_CONSTEXPR ~S() {}
+  };
+  S s; // cxx11-warning {{global destructor}}
+
+  struct T {
+CPP20_CONSTEXPR ~T() { if (b) {} }
+bool b;
+  };
+  T t; // expected-warning {{global destructor}}
+#undef CPP20_CONSTEXPR
+}
Index: clang/test/SemaCXX/warn-exit-time-destructors.cpp
===
--- clang/test/SemaCXX/warn-exit-time-destructors.cpp
+++ clang/test/SemaCXX/warn-exit-time-destructors.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wexit-time-destructors %s -verify
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wexit-time-destructors %s -verify=expected,cxx11
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wexit-time-destructors %s -verify=expected
 
 namespace test1 {
   struct A { ~A(); };
@@ -48,3 +49,22 @@
 struct A { ~A(); };
 [[clang::no_destroy]] A a; // no warning
 }
+
+namespace test5 {
+#if __cplusplus >= 202002L
+#define CPP20_CONSTEXPR constexpr
+#else
+#define CPP20_CONSTEXPR
+#endif
+  struct S {
+CPP20_CONSTEXPR ~S() {}
+  };
+  S s; // c

[PATCH] D149456: Basic documentation of -mrecip=... option

[Shivam Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG85ed0fb5037b: Basic documentation of -mrecip=... option 
(authored by tim.schmielau, committed by xgupta).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149456/new/

https://reviews.llvm.org/D149456

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3959,8 +3959,12 @@
 def mno_implicit_float : Flag<["-"], "mno-implicit-float">, Group,
   HelpText<"Don't generate implicit floating point or vector instructions">;
 def mimplicit_float : Flag<["-"], "mimplicit-float">, Group;
-def mrecip : Flag<["-"], "mrecip">, Group;
+def mrecip : Flag<["-"], "mrecip">, Group,
+  HelpText<"Equivalent to '-mrecip=all'">;
 def mrecip_EQ : CommaJoined<["-"], "mrecip=">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Control use of approximate reciprocal and reciprocal square root 
instructions followed by  iterations of "
+   "Newton-Raphson refinement. "
+   " = ( ['!'] ['vec-'] ('rcp'|'sqrt') [('h'|'s'|'d')] [':'] 
) | 'all' | 'default' | 'none'">,
   MarshallingInfoStringVector>;
 def mprefer_vector_width_EQ : Joined<["-"], "mprefer-vector-width=">, 
Group, Flags<[CC1Option]>,
   HelpText<"Specifies preferred vector width for auto-vectorization. Defaults 
to 'none' which allows target specific decisions.">,


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3959,8 +3959,12 @@
 def mno_implicit_float : Flag<["-"], "mno-implicit-float">, Group,
   HelpText<"Don't generate implicit floating point or vector instructions">;
 def mimplicit_float : Flag<["-"], "mimplicit-float">, Group;
-def mrecip : Flag<["-"], "mrecip">, Group;
+def mrecip : Flag<["-"], "mrecip">, Group,
+  HelpText<"Equivalent to '-mrecip=all'">;
 def mrecip_EQ : CommaJoined<["-"], "mrecip=">, Group, Flags<[CC1Option]>,
+  HelpText<"Control use of approximate reciprocal and reciprocal square root instructions followed by  iterations of "
+   "Newton-Raphson refinement. "
+   " = ( ['!'] ['vec-'] ('rcp'|'sqrt') [('h'|'s'|'d')] [':'] ) | 'all' | 'default' | 'none'">,
   MarshallingInfoStringVector>;
 def mprefer_vector_width_EQ : Joined<["-"], "mprefer-vector-width=">, Group, Flags<[CC1Option]>,
   HelpText<"Specifies preferred vector width for auto-vectorization. Defaults to 'none' which allows target specific decisions.">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149405: [Clang][Doc] Added an open project for improving command line docs

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D149405#4305856 , @aaron.ballman 
wrote:

> Thank you, this is a great suggestion! Just some minor wording changes, but 
> otherwise LGTM.

Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149405/new/

https://reviews.llvm.org/D149405

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149405: [Clang][Doc] Added an open project for improving command line docs

[Shivam Gupta via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
xgupta marked an inline comment as done.
Closed by commit rGa70485493803: [Clang][Doc] Added an open project for 
improving command line docs (authored by xgupta).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149405/new/

https://reviews.llvm.org/D149405

Files:
  clang/www/OpenProjects.html


Index: clang/www/OpenProjects.html
===
--- clang/www/OpenProjects.html
+++ clang/www/OpenProjects.html
@@ -38,6 +38,8 @@
   documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDocs.td";>
   diagnostic group flags (adding code examples of what is diagnosed, or
   other relevant information), or
+  documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td";>
+  command line options, or
   help with completing other missing documentation.
 
 These projects are independent of each other.


Index: clang/www/OpenProjects.html
===
--- clang/www/OpenProjects.html
+++ clang/www/OpenProjects.html
@@ -38,6 +38,8 @@
   documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDocs.td";>
   diagnostic group flags (adding code examples of what is diagnosed, or
   other relevant information), or
+  documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td";>
+  command line options, or
   help with completing other missing documentation.
 
 These projects are independent of each other.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149405: [Clang][Doc] Added an open project for improving command line docs

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 517996.
xgupta added a comment.

address comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149405/new/

https://reviews.llvm.org/D149405

Files:
  clang/www/OpenProjects.html


Index: clang/www/OpenProjects.html
===
--- clang/www/OpenProjects.html
+++ clang/www/OpenProjects.html
@@ -38,6 +38,8 @@
   documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDocs.td";>
   diagnostic group flags (adding code examples of what is diagnosed, or
   other relevant information), or
+  documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td";>
+  command line options, or
   help with completing other missing documentation.
 
 These projects are independent of each other.


Index: clang/www/OpenProjects.html
===
--- clang/www/OpenProjects.html
+++ clang/www/OpenProjects.html
@@ -38,6 +38,8 @@
   documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDocs.td";>
   diagnostic group flags (adding code examples of what is diagnosed, or
   other relevant information), or
+  documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td";>
+  command line options, or
   help with completing other missing documentation.
 
 These projects are independent of each other.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149405: [Clang][Doc] Added an open project for improving command line docs

[Shivam Gupta via Phabricator via cfe-commits]

xgupta created this revision.
xgupta added a reviewer: aaron.ballman.
Herald added a project: All.
xgupta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

There seems to be a lot of documentation is missing
for different command line flags uses. 
This create confusion with same looking options, better to 
document clearly their uses.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149405

Files:
  clang/www/OpenProjects.html


Index: clang/www/OpenProjects.html
===
--- clang/www/OpenProjects.html
+++ clang/www/OpenProjects.html
@@ -38,6 +38,10 @@
   documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDocs.td";>
   diagnostic group flags (adding code examples of what is diagnosed, or
   other relevant information), or
+  improve existing and add missing documentation for
+  https://clang.llvm.org/docs/ClangCommandLineReference.html";>
+  clang command line flags, generated from https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td";>
+  clang/Driver/Options.td,
   help with completing other missing documentation.
 
 These projects are independent of each other.


Index: clang/www/OpenProjects.html
===
--- clang/www/OpenProjects.html
+++ clang/www/OpenProjects.html
@@ -38,6 +38,10 @@
   documenting https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDocs.td";>
   diagnostic group flags (adding code examples of what is diagnosed, or
   other relevant information), or
+  improve existing and add missing documentation for
+  https://clang.llvm.org/docs/ClangCommandLineReference.html";>
+  clang command line flags, generated from https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td";>
+  clang/Driver/Options.td,
   help with completing other missing documentation.
 
 These projects are independent of each other.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D148439#4298606 , @aaron.ballman 
wrote:

> LGTM, but please add a release note when landing the changes.

Thanks for the review, added a release note for change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG62eec1584d2c: [clang-rename] Exit gracefully when no input 
provided (authored by xgupta).

Changed prior to commit:
  https://reviews.llvm.org/D148439?vs=514842&id=517178#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

Files:
  clang/docs/ReleaseNotes.rst
  clang/test/clang-rename/NonExistFile.cpp
  clang/tools/clang-rename/ClangRename.cpp


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -229,6 +229,10 @@
 Tool.applyAllReplacements(Rewrite);
 for (const auto &File : Files) {
   auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: " << File << " does not exist.\n";
+return 1;
+  }
   const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
   Rewrite.getEditBuffer(ID).write(outs());
 }
Index: clang/test/clang-rename/NonExistFile.cpp
===
--- /dev/null
+++ clang/test/clang-rename/NonExistFile.cpp
@@ -0,0 +1,2 @@
+// RUN: not clang-rename -offset=0 -new-name=bar non-existing-file 2>&1 | 
FileCheck %s
+// CHECK: clang-rename: non-existing-file does not exist.
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -235,6 +235,8 @@
 Bug Fixes in This Version
 -
 
+- Fix segfault while running clang-rename on a non existing file.
+  (`#36471 `_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 `_)


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -229,6 +229,10 @@
 Tool.applyAllReplacements(Rewrite);
 for (const auto &File : Files) {
   auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: " << File << " does not exist.\n";
+return 1;
+  }
   const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
   Rewrite.getEditBuffer(ID).write(outs());
 }
Index: clang/test/clang-rename/NonExistFile.cpp
===
--- /dev/null
+++ clang/test/clang-rename/NonExistFile.cpp
@@ -0,0 +1,2 @@
+// RUN: not clang-rename -offset=0 -new-name=bar non-existing-file 2>&1 | FileCheck %s
+// CHECK: clang-rename: non-existing-file does not exist.
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -235,6 +235,8 @@
 Bug Fixes in This Version
 -
 
+- Fix segfault while running clang-rename on a non existing file.
+  (`#36471 `_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 `_)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Ping for reivew.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: clang/tools/clang-rename/ClangRename.cpp:233
+  if (!Entry) {
+errs() << "clang-rename: input file does not exist.\n";
+return 1;

kbobyrev wrote:
> It is worth including the filename in the error message, otherwise it won't 
> be possible to understand which one is missing (there can be multiple IIRC, 
> right?).
> 
> Also, it's better to put this check to the top of main, where the `OP` if 
> first declared, since this is a sanity check and we want to fail if the 
> inputs are corrupted.
Thanks for the suggestion, I agree, I updated the error message regarding the 
filename. 

But I am not sure running a similar `for` loop again should be the correct way.
So just sharing the diff before committing, WDYT -

```
diff --git a/clang/tools/clang-rename/ClangRename.cpp 
b/clang/tools/clang-rename/ClangRename.cpp
index 24c9d8521..bc777cc01 100644
--- a/clang/tools/clang-rename/ClangRename.cpp
+++ b/clang/tools/clang-rename/ClangRename.cpp
@@ -106,6 +106,17 @@ int main(int argc, const char **argv) {
   }
   tooling::CommonOptionsParser &OP = ExpectedParser.get();
 
+  auto Files = OP.getSourcePathList();
+  tooling::RefactoringTool Tool(OP.getCompilations(), Files);
+auto &FileMgr = Tool.getFiles();
+  for (const auto &File : Files) {
+  auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: " << File << " does not exist.\n";
+return 1;
+  }
+}
+
   if (!Input.empty()) {
 // Populate QualifiedNames and NewNames from a YAML file.
 ErrorOr> Buffer =
@@ -162,8 +173,6 @@ int main(int argc, const char **argv) {
 return 1;
   }
 
-  auto Files = OP.getSourcePathList();
-  tooling::RefactoringTool Tool(OP.getCompilations(), Files);
   tooling::USRFindingAction FindingAction(SymbolOffsets, QualifiedNames, 
Force);
   Tool.run(tooling::newFrontendActionFactory(&FindingAction).get());
   const std::vector> &USRList =
@@ -222,7 +231,6 @@ int main(int argc, const char **argv) {
 DiagnosticsEngine Diagnostics(
 IntrusiveRefCntPtr(new DiagnosticIDs()), &*DiagOpts,
 &DiagnosticPrinter, false);
-auto &FileMgr = Tool.getFiles();
 SourceManager Sources(Diagnostics, FileMgr);
 Rewriter Rewrite(Sources, DefaultLangOptions);
 
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 514842.
xgupta marked an inline comment as done.
xgupta added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

Files:
  clang/test/clang-rename/NonExistFile.cpp
  clang/tools/clang-rename/ClangRename.cpp


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -229,6 +229,10 @@
 Tool.applyAllReplacements(Rewrite);
 for (const auto &File : Files) {
   auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: " << File << " does not exist.\n";
+return 1;
+  }
   const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
   Rewrite.getEditBuffer(ID).write(outs());
 }
Index: clang/test/clang-rename/NonExistFile.cpp
===
--- /dev/null
+++ clang/test/clang-rename/NonExistFile.cpp
@@ -0,0 +1,2 @@
+// RUN: not clang-rename -offset=0 -new-name=bar non-existing-file 2>&1 | 
FileCheck %s
+// CHECK: clang-rename: non-existing-file does not exist.


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -229,6 +229,10 @@
 Tool.applyAllReplacements(Rewrite);
 for (const auto &File : Files) {
   auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: " << File << " does not exist.\n";
+return 1;
+  }
   const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
   Rewrite.getEditBuffer(ID).write(outs());
 }
Index: clang/test/clang-rename/NonExistFile.cpp
===
--- /dev/null
+++ clang/test/clang-rename/NonExistFile.cpp
@@ -0,0 +1,2 @@
+// RUN: not clang-rename -offset=0 -new-name=bar non-existing-file 2>&1 | FileCheck %s
+// CHECK: clang-rename: non-existing-file does not exist.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D148439#4272904 , @kbobyrev wrote:

> Oh, wait, I'm sorry, I didn't look into it closely :( Yeah, the `Input` file 
> is not really needed, most of the time the users of `clang-rename` (not sure 
> there are many with `clangd` being developed) use CLI flags for that.
>
> It's better to revert this patch. My bad, should've looked a bit closer into 
> this.

No issue, I also consider it a straightforward fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 514262.
xgupta added a comment.

adjust the return and add test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

Files:
  clang/test/clang-rename/NonExistFile.cpp
  clang/tools/clang-rename/ClangRename.cpp


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -229,6 +229,10 @@
 Tool.applyAllReplacements(Rewrite);
 for (const auto &File : Files) {
   auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: input file does not exist.\n";
+return 1;
+  }
   const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
   Rewrite.getEditBuffer(ID).write(outs());
 }
Index: clang/test/clang-rename/NonExistFile.cpp
===
--- /dev/null
+++ clang/test/clang-rename/NonExistFile.cpp
@@ -0,0 +1,2 @@
+// RUN: not clang-rename -offset=0 -new-name=plop asdasd 2>&1 | FileCheck %s
+// CHECK: clang-rename: input file does not exist.


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -229,6 +229,10 @@
 Tool.applyAllReplacements(Rewrite);
 for (const auto &File : Files) {
   auto Entry = FileMgr.getFile(File);
+  if (!Entry) {
+errs() << "clang-rename: input file does not exist.\n";
+return 1;
+  }
   const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
   Rewrite.getEditBuffer(ID).write(outs());
 }
Index: clang/test/clang-rename/NonExistFile.cpp
===
--- /dev/null
+++ clang/test/clang-rename/NonExistFile.cpp
@@ -0,0 +1,2 @@
+// RUN: not clang-rename -offset=0 -new-name=plop asdasd 2>&1 | FileCheck %s
+// CHECK: clang-rename: input file does not exist.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D148439#4272813 , @dyung wrote:

> This change is also causing 33 test failures on a build bot 
> https://lab.llvm.org/buildbot/#/builders/139/builds/39267

Yes, checking for the reason, and honestly I never tried to run clang-rename 
for its uses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D148439#4272810 , @dyung wrote:

> Is there a reason a test was not added with this change?

Sorry, I missed that. Adding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
xgupta marked an inline comment as done.
Closed by commit rG726199146a0b: [clang-rename] Exit gracefully when no input 
provided (authored by xgupta).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

Files:
  clang/tools/clang-rename/ClangRename.cpp


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -126,6 +126,9 @@
 SymbolOffsets.push_back(Info.Offset);
   NewNames.push_back(Info.NewName);
 }
+  } else {
+errs() << "clang-rename: input must be provided.\n";
+return 1;
   }
 
   // Check the arguments for correctness.


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -126,6 +126,9 @@
 SymbolOffsets.push_back(Info.Offset);
   NewNames.push_back(Info.NewName);
 }
+  } else {
+errs() << "clang-rename: input must be provided.\n";
+return 1;
   }
 
   // Check the arguments for correctness.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta marked an inline comment as done.
xgupta added inline comments.



Comment at: clang/tools/clang-rename/ClangRename.cpp:130
+  } else {
+errs() << "clang-rename: No input provided.\n";
+return 1;

kbobyrev wrote:
> Probably something like "input must be provided" would be more understandable 
> by the user, but this is also fine.
Thanks for the suggestion!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 514093.
xgupta added a comment.

address comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148439/new/

https://reviews.llvm.org/D148439

Files:
  clang/tools/clang-rename/ClangRename.cpp


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -126,6 +126,9 @@
 SymbolOffsets.push_back(Info.Offset);
   NewNames.push_back(Info.NewName);
 }
+  } else {
+errs() << "clang-rename: input must be provided.\n";
+return 1;
   }
 
   // Check the arguments for correctness.


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -126,6 +126,9 @@
 SymbolOffsets.push_back(Info.Offset);
   NewNames.push_back(Info.NewName);
 }
+  } else {
+errs() << "clang-rename: input must be provided.\n";
+return 1;
   }
 
   // Check the arguments for correctness.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148439: [clang-rename] Exit gracefully when no input provided

[Shivam Gupta via Phabricator via cfe-commits]

xgupta created this revision.
xgupta added reviewers: vmiklos, alexander-shaposhnikov, kbobyrev, 
aaron.ballman.
Herald added a project: All.
xgupta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

clang-rename on a non existing file segfaults

Command to run - 
$ clang-rename -offset=0 -new-name=plop asdasd

Error while processing llvm-project/asdasd.
clang-rename: llvm-project/llvm/include/llvm/Support/ErrorOr.h:237: 
llvm::ErrorOr::storage_type* llvm::ErrorOr::getStorage() 
[with T = const clang::FileEntry*; llvm::ErrorOr::storage_type = const 
clang::FileEntry*]: 
Assertion `!HasError && "Cannot get value when an error exists!"' failed.

[1]827497 IOT instruction  clang-rename -offset=0 -new-name=plop asdasd


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148439

Files:
  clang/tools/clang-rename/ClangRename.cpp


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -126,6 +126,9 @@
 SymbolOffsets.push_back(Info.Offset);
   NewNames.push_back(Info.NewName);
 }
+  } else {
+errs() << "clang-rename: No input provided.\n";
+return 1;
   }
 
   // Check the arguments for correctness.


Index: clang/tools/clang-rename/ClangRename.cpp
===
--- clang/tools/clang-rename/ClangRename.cpp
+++ clang/tools/clang-rename/ClangRename.cpp
@@ -126,6 +126,9 @@
 SymbolOffsets.push_back(Info.Offset);
   NewNames.push_back(Info.NewName);
 }
+  } else {
+errs() << "clang-rename: No input provided.\n";
+return 1;
   }
 
   // Check the arguments for correctness.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

gentle ping for review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta marked 5 inline comments as done.
xgupta added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13635
+  if (ECDHS && ECDHS->getInitVal() != 0 && ECDHS->getInitVal() != 1)
+EnumConstantInBoolContext = true;
+}

nickdesaulniers wrote:
> Hmm...I wonder if we now care about which `ExprResult` contained an enum 
> constant in bool context?
IIUC you mean EnumConstantInBoolContext is not required, but then test cases 
start failing if I will not use it in diagnoseLogicalInsteadOfBitwise above.
Like this - 

$ build/bin/llvm-lit -v clang/test/Sema

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 52: use of logical '||' with constant operand
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 56: use of logical '||' with constant operand
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 68: use of logical '&&' with constant operand
error: 'note' diagnostics seen but not expected: 
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 52: use '|' for a bitwise operation
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 56: use '|' for a bitwise operation
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 68: use '&' for a bitwise operation
  File 
/home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c 
Line 68: remove constant to silence this warning
7 errors generated.

--
Failed Tests (1):
  Clang :: Sema/warn-int-in-bool-context.c


Testing Time: 0.04s
  Failed: 1



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 495375.
xgupta added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,6 +44,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
@@ -66,6 +69,8 @@
// expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant operand

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 495122.
xgupta added a comment.

remove old code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,6 +44,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
@@ -66,6 +69,8 @@
// expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant operand}

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 495116.
xgupta added a comment.

use function to avoid code duplication


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,6 +44,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
@@ -66,6 +69,8 @@
// expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||'

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13611-13653
 if (RHS.get()->EvaluateAsInt(EVResult, Context)) {
   llvm::APSInt Result = EVResult.Val.getInt();
   if ((getLangOpts().Bool && !RHS.get()->getType()->isBooleanType() &&
!RHS.get()->getExprLoc().isMacroID()) ||
   (Result != 0 && Result != 1)) {
 Diag(Loc, diag::warn_logical_instead_of_bitwise)
 << RHS.get()->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");

nickdesaulniers wrote:
> There seems to be a lot of duplication between the two; should we move these 
> into a static function that's called twice? Perhaps Op1 and Op2 would be 
> better identifiers than LHS RHS in that case?
I have tried many ways, but couldn't figure out a way to do it with function, 
unrelated test cases started failing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 494631.
xgupta added a comment.

remov blank


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/C/drs/dr4xx.c
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp
  clang/test/Parser/cxx2a-concept-declaration.cpp
  clang/test/Sema/exprs.c
  clang/test/SemaCXX/expressions.cpp
  clang/test/SemaCXX/warn-unsequenced.cpp
  clang/test/SemaTemplate/dependent-expr.cpp

Index: clang/test/SemaTemplate/dependent-expr.cpp
===
--- clang/test/SemaTemplate/dependent-expr.cpp
+++ clang/test/SemaTemplate/dependent-expr.cpp
@@ -43,7 +43,9 @@
 
 namespace PR7724 {
   template int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+  // expected-note {{use '&' for a bitwise operation}} \
+  // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -76,15 +76,37 @@
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+  // cxx11-note@-1 {{use '&' for a bitwise operation}}
+  // cxx11-note@-2 {{remove constant to silence this warning}}
+  // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-4 {{use '&' for a bitwise operation}}
+  // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+  // cxx11-note@-3 {{use '&' for a bitwise operation}}
+  // cxx11-note@-4 {{remove constant to silence this warning}}
+  // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+  // cxx17-note@-6 {{use '&' for a bitwise operation}}
+  // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+  // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx11-note@-3 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+  // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+  // cxx11-note@-1 {{use '|' for a bitwise operation}}
+  // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+  // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
Index: clang/test/SemaCXX/expressions.cpp
===
--- clang/test/SemaCXX/expressions.cpp
+++ clang/test/SemaCXX/expressions.cpp
@@ -44,6 +44,9 @@
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
// expected-note {{use '&' for a bitwise operation}} \
// expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+   // expected-note {{use '&' for a bitwise operation}} \
+   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
@@ -66,6 +69,8 @@
// expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
// expected-no