[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

@aaron.ballman - can this land for Clang14, or does it have wait for 15?

Are there any other reviewers that can approve this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp:12
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 
'double *' can be declared 'const'
+  // CHECK-FIXES: const
+}

LegalizeAdulthood wrote:
> Shouldn't this validate that the `const` was placed in the correct position?
> e.g. `const double *` is a different meaning from `double *const`
> 
> Apply to all the other `CHECK-FIXES` as well
Can we have the checker merged in first, then we can worry about the automatic 
fixer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

+1

Let's get this in :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#3048763 , @JonasToth wrote:

> ping

Hi Jonas,

Using patch32 in production (on top of "release/13.x" branch) ever since it was 
made available. No crashes.

Only one false positive reported, which I'm not sure even is a false positive, 
is when I used std::visit and passed in a reference to an object as the first 
argument, the checker recommended I annotate the variable as const, even though 
std::visit takes the argument by &&, and changing it to const would have 
resulted in a miscompile. I think there's also some difference in how 
std::visit is implemented between various standard C++ libraries, so not sure 
if that's a special case or something more general.

Quoting from memory:

   std::variant theThing;
  
   class Parameter
   {
  int operator()(const Foo& foo)
  {
   ...
  }
  int operator()(const Bar& bar)
  {
   ...
  }
   } param;  // cannot be const, but the checker warns that it could be const
  
  std::visit(param, theThing)

I'm hoping to have some time next week to distill a minimal test case.

Thank you,
florin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Using the checker now in our production BuildBot - no crashes and no false 
positives. Can't say if there are false negatives, but at any rate the checker 
is better than most colleagues at finding what should be declared const.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

The diff was generated with format-patch, so can be applied with "git am".

The envelope:

> commit d98d336b452876cfbc32ad1b76319912c45b646b (HEAD -> 
> readability-variable-name-length-main)
> Author: Florin Iucha 
> Date:   Mon Mar 1 00:57:01 2021 -0500
>
>   Add a checker for variable name length
>   
>   It requires regular variables and parameters names to be at least
>   three characters long, and loop variables and exception names to be at
>   least two characters long. Exceptions to these minimums can be added
>   via regular expressions.




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

https://reviews.llvm.org/D97753

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 365891.
0x8000- added a comment.

One more format nit.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s readability-identifier-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept {
+  int val;
+};
+
+struct simpleexcept {
+  int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-identifier-length]
+{
+  int i = 5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]
+
+  int jj = z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable name 'jj' is too short, expected at least 3 characters [readability-identifier-length]
+
+  for (int m = 0; m < 5; ++m)
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-identifier-length]
+  {
+doIt();
+  }
+
+  try {
+doIt();
+  } catch (const myexcept )
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-identifier-length]
+  {
+doIt();
+  }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by default configuration
+{
+  int var = 5;
+
+  for (int i = 0; i < 42; ++i) // 'i' is default allowed, for historical reasons
+  {
+doIt();
+  }
+
+  for (int kk = 0; kk < 42; ++kk) {
+doIt();
+  }
+
+  try {
+doIt();
+  } catch (const simpleexcept ) // ignored by default configuration
+  {
+doIt();
+  } catch (const myexcept ) {
+doIt();
+  }
+
+  int x = 5; // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
@@ -0,0 +1,122 @@
+.. title:: clang-tidy - readability-identifier-length
+
+readability-identifier-length
+=
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumParameterNameLength`, :option:`IgnoredParameterNames`
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumExceptionNameLength`, :option:`IgnoredExceptionVariableNames`
+
+.. option:: MinimumVariableNameLength
+
+All variables (other than loop counter, exception names and function
+parameters) are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+ int doubler(int x)   // warns that x is too short
+ {
+return 2 * x;
+ }
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredVariableNames
+
+Specifies a regular expression for variable names that are
+to be ignored. The default value is empty, thus no names are ignored.
+
+.. option:: MinimumParameterNameLength
+
+All function parameter names are expected to have a length of at least
+`MinimumParameterNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredParameterNames
+
+Specifies a regular expression for parameters that are to be ignored.
+The default value is `^[n]$` for historical reasons.
+
+.. 

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 365879.
0x8000- added a comment.

Reformatted the test file.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s readability-identifier-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept {
+  int val;
+};
+
+struct simpleexcept {
+  int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-identifier-length]
+{
+  int i = 5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]
+
+  int jj = z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable name 'jj' is too short, expected at least 3 characters [readability-identifier-length]
+
+  for (int m = 0; m < 5; ++m)
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-identifier-length]
+  {
+doIt();
+  }
+
+  try {
+doIt();
+  } catch (const myexcept )
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-identifier-length]
+  {
+doIt();
+  }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by default configuration
+{
+  int var = 5;
+
+  for (int i = 0; i < 42; ++i) // 'i' is default allowed, for historical reasons
+  {
+doIt();
+  }
+
+  for (int kk = 0; kk < 42; ++kk) {
+doIt();
+  }
+
+  try {
+doIt();
+  } catch (const simpleexcept ) // ignored by default configuration
+  {
+doIt();
+  } catch (const myexcept ) {
+doIt();
+  }
+
+  int x = 5; // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
@@ -0,0 +1,122 @@
+.. title:: clang-tidy - readability-identifier-length
+
+readability-identifier-length
+=
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumParameterNameLength`, :option:`IgnoredParameterNames`
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumExceptionNameLength`, :option:`IgnoredExceptionVariableNames`
+
+.. option:: MinimumVariableNameLength
+
+All variables (other than loop counter, exception names and function
+parameters) are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+ int doubler(int x)   // warns that x is too short
+ {
+return 2 * x;
+ }
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredVariableNames
+
+Specifies a regular expression for variable names that are
+to be ignored. The default value is empty, thus no names are ignored.
+
+.. option:: MinimumParameterNameLength
+
+All function parameter names are expected to have a length of at least
+`MinimumParameterNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredParameterNames
+
+Specifies a regular expression for parameters that are to be ignored.
+The default value is `^[n]$` for historical reasons.
+

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

@aaron.ballman - thank you for the review; please submit on my behalf.


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

https://reviews.llvm.org/D97753

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 365852.
0x8000- added a comment.

Removed extra 'const' and parentheses that are not congruent with the accepted 
code style.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s readability-identifier-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept
+{
+   int val;
+};
+
+struct simpleexcept
+{
+   int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-identifier-length]
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]
+
+   int jj = z;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-identifier-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-identifier-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-identifier-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by default configuration
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const simpleexcept& e) // ignored by default configuration
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+
+   int x = 5;  // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
@@ -0,0 +1,122 @@
+.. title:: clang-tidy - readability-identifier-length
+
+readability-identifier-length
+=
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumParameterNameLength`, :option:`IgnoredParameterNames`
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumExceptionNameLength`, :option:`IgnoredExceptionVariableNames`
+
+.. option:: MinimumVariableNameLength
+
+All variables (other than loop counter, exception names and function
+parameters) are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+ int doubler(int x)   // warns that x is too short
+ {
+return 2 * x;
+ }
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredVariableNames
+
+Specifies a regular expression for variable names that are
+to be ignored. The default value is empty, thus no names are ignored.
+
+.. option:: MinimumParameterNameLength
+
+All function parameter names are expected to have a length of at least
+`MinimumParameterNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredParameterNames
+

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 365651.
0x8000- added a comment.

Clean diff against origin/main


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s readability-identifier-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept
+{
+   int val;
+};
+
+struct simpleexcept
+{
+   int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-identifier-length]
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]
+
+   int jj = z;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-identifier-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-identifier-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-identifier-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by default configuration
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const simpleexcept& e) // ignored by default configuration
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+
+   int x = 5;  // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
@@ -0,0 +1,122 @@
+.. title:: clang-tidy - readability-identifier-length
+
+readability-identifier-length
+=
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumParameterNameLength`, :option:`IgnoredParameterNames`
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumExceptionNameLength`, :option:`IgnoredExceptionVariableNames`
+
+.. option:: MinimumVariableNameLength
+
+All variables (other than loop counter, exception names and function
+parameters) are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+ int doubler(int x)   // warns that x is too short
+ {
+return 2 * x;
+ }
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredVariableNames
+
+Specifies a regular expression for variable names that are
+to be ignored. The default value is empty, thus no names are ignored.
+
+.. option:: MinimumParameterNameLength
+
+All function parameter names are expected to have a length of at least
+`MinimumParameterNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredParameterNames
+
+Specifies a regular expression for parameters that are 

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 365622.
0x8000- added a comment.

Rebased on main and renamed from readability-variable-length to 
readability-identifier-length.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierLengthCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-length.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s readability-identifier-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-identifier-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept
+{
+   int val;
+};
+
+struct simpleexcept
+{
+   int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-identifier-length]
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]
+
+   int jj = z;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-identifier-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-identifier-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-identifier-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by default configuration
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const simpleexcept& e) // ignored by default configuration
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+
+   int x = 5;  // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-length.rst
@@ -0,0 +1,122 @@
+.. title:: clang-tidy - readability-identifier-length
+
+readability-identifier-length
+=
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumParameterNameLength`, :option:`IgnoredParameterNames`
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumExceptionNameLength`, :option:`IgnoredExceptionVariableNames`
+
+.. option:: MinimumVariableNameLength
+
+All variables (other than loop counter, exception names and function
+parameters) are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+ int doubler(int x)   // warns that x is too short
+ {
+return 2 * x;
+ }
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredVariableNames
+
+Specifies a regular expression for variable names that are
+to be ignored. The default value is empty, thus no names are ignored.
+
+.. option:: MinimumParameterNameLength
+
+All function parameter names are expected to have a length of at least
+`MinimumParameterNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: 

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+CheckFactories.registerCheck(
+"readability-variable-length");
   }

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > Is there a reason this should be restricted to variables? For 
> > > > > example, wouldn't the same functionality be useful for type names, or 
> > > > > dare I say it, even macro names? I'm wondering if this should be 
> > > > > `readability-identifier-length` to be more generic.
> > > > I consider those to be in separate namespaces. I suppose we could have 
> > > > a single checker with multiple rules, but on the other hand I don't 
> > > > want to combine too many things into one, just because they share the 
> > > > "compare length" dimension.
> > > I see where you're coming from, but I come down on the other side. 
> > > Running a check is expensive (especially on Windows where process 
> > > creation can be really slow), so having multiple checks that traverse the 
> > > AST gives worse performance than having a single check that only 
> > > traverses the AST once. I'd rather see related functionality grouped 
> > > together and use options to control behavior when it's a natural fit to 
> > > do so.
> > > 
> > > I should note that I don't mean *you* have to implement this other 
> > > functionality (as part of this patch or otherwise). Just that I think we 
> > > should leave the check name ambiguous enough that we could grow it to do 
> > > that work in the future.
> > > 
> > > WDYT?
> > Right - that's a good point. But I'm wondering the other way; maybe the 
> > bigger check will subsume this one, and this one will become just an alias 
> > for the bigger check?
> > 
> > So I'm -0.1 on using the "bigger name" for the limited functionality, but 
> > if one more vote comes in saying to go readability-identifier-length I'll 
> > rename this (and add explicitly the scope limits in the documentation.)
> > Right - that's a good point. But I'm wondering the other way; maybe the 
> > bigger check will subsume this one, and this one will become just an alias 
> > for the bigger check?
> 
> The downside to that approach is that the alias is a bit confusing until its 
> deprecation period ends and we remove it. However, that's not a huge 
> downside., so I don't insist on the name change if you're resistant to it.
I can do the rename, but my intuition (informed somewhat by a few years in the 
industry) is that outside of T as template argument and X for X-Macros, there 
aren't many single or even two letter class names or structures out there 
outside of intentionally obfuscated code - thus I don't think there will be a 
need to have readability-identifier-length produced.

To avoid potential churn and the hassle with deprecation I'll make the change 
though.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp:44
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > What in the configuration causes `n` to be ignored?
> > It is ignored by the default configuration. Search for 
> > "DefaultIgnoredParameterNames" above.
> Ah, the comment tripped me up -- can you say `ignored via default 
> configuration` like below to make it more clear?
Sorry about that; fixed in next patch.


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

https://reviews.llvm.org/D97753

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+CheckFactories.registerCheck(
+"readability-variable-length");
   }

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > Is there a reason this should be restricted to variables? For example, 
> > > wouldn't the same functionality be useful for type names, or dare I say 
> > > it, even macro names? I'm wondering if this should be 
> > > `readability-identifier-length` to be more generic.
> > I consider those to be in separate namespaces. I suppose we could have a 
> > single checker with multiple rules, but on the other hand I don't want to 
> > combine too many things into one, just because they share the "compare 
> > length" dimension.
> I see where you're coming from, but I come down on the other side. Running a 
> check is expensive (especially on Windows where process creation can be 
> really slow), so having multiple checks that traverse the AST gives worse 
> performance than having a single check that only traverses the AST once. I'd 
> rather see related functionality grouped together and use options to control 
> behavior when it's a natural fit to do so.
> 
> I should note that I don't mean *you* have to implement this other 
> functionality (as part of this patch or otherwise). Just that I think we 
> should leave the check name ambiguous enough that we could grow it to do that 
> work in the future.
> 
> WDYT?
Right - that's a good point. But I'm wondering the other way; maybe the bigger 
check will subsume this one, and this one will become just an alias for the 
bigger check?

So I'm -0.1 on using the "bigger name" for the limited functionality, but if 
one more vote comes in saying to go readability-identifier-length I'll rename 
this (and add explicitly the scope limits in the documentation.)



Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > Should it be possible to enforce parameters differently than local 
> > > variables? (It seems a bit odd that we'd allow you to specify exception 
> > > variable length separate from loops but not function parameters separate 
> > > from locals.)
> > Exceptions and Loops historically are "throw-away" / "don't care" / 
> > "one-off" variables. Parameter names are names, and they are the interface 
> > between the outside of the routine and the processing inside; other than 
> > historical, I don't see good arguments (sic) to allow single-letter 
> > parameter names.
> > 
> > Note that this check will be quite noisy on a legacy code base, and people 
> > will find little reason to invest the work to remove the warnings. But if 
> > somebody starts something new and want to enforce some quality attributes, 
> > it is the right tool at the right time. There will be new projects starting 
> > in 2021 and 2022 that could benefit from this, I hope.
> > Exceptions and Loops historically are "throw-away" / "don't care" / 
> > "one-off" variables. Parameter names are names, and they are the interface 
> > between the outside of the routine and the processing inside; other than 
> > historical, I don't see good arguments (sic) to allow single-letter 
> > parameter names.
> 
> I largely agree with you, but there are definitely cases where single-letter 
> parameter names are extremely common. For example, coordinates are often `x`, 
> `y`, and `z`, colors are often `r`, `g`, and `b` (among many others), and 
> physics applications often do things like `f = m * a` with their variables. 
> Also, there are cases where the parameter names are often not super 
> meaningful, like with functions `min` and `max` so the parameter names may be 
> quite short.
Indeed - I have changed the code to meet this capability; now parameters are 
controlled independently of the regular variables.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp:44
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{

aaron.ballman wrote:
> What in the configuration causes `n` to be ignored?
It is ignored by the default configuration. Search for 
"DefaultIgnoredParameterNames" above.


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

https://reviews.llvm.org/D97753

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 365016.
0x8000- added a comment.

Updated documentation for all options.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s readability-variable-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-variable-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept
+{
+   int val;
+};
+
+struct simpleexcept
+{
+   int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-variable-length]
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-variable-length]
+
+   int jj = z;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-variable-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const simpleexcept& e) // ignored via default configuration
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+
+   int x = 5;  // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
@@ -0,0 +1,122 @@
+.. title:: clang-tidy - readability-variable-length
+
+readability-variable-length
+===
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumParameterNameLength`, :option:`IgnoredParameterNames`
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumExceptionNameLength`, :option:`IgnoredExceptionVariableNames`
+
+.. option:: MinimumVariableNameLength
+
+All variables (other than loop counter, exception names and function
+parameters) are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+ int doubler(int x)   // warns that x is too short
+ {
+return 2 * x;
+ }
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredVariableNames
+
+Specifies a regular expression for variable names that are
+to be ignored. The default value is empty, thus no names are ignored.
+
+.. option:: MinimumParameterNameLength
+
+All function parameter names are expected to have a length of at least
+`MinimumParameterNameLength` (default is `3`). Setting it to `0` or `1`
+disables the check entirely.
+
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
+
+.. option:: IgnoredParameterNames
+
+Specifies a regular expression for parameters that are to be ignored.
+The default 

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2933179 , @JonasToth wrote:

> In D54943#2633408 , @tiagoma wrote:
>
>> Can we get this in? I work in Microsoft Office and we have been using this 
>> checker and it works great! There are a couple of issues with it and I would 
>> like to contribute fixes.
>
> YES. I WILL WORK ON THIS NOW.

Great news Jonas! Thank you!

> After s much time has passed I think i have finally time again to bring 
> this check over the line. I will invest at least every sunday from now on, 
> until its done :)
>
> This is my initial work-list I would like to fix before this check can be 
> merged:
>
> - rebase to current master (obviously)
> - fix `clang-apply-replacements` duplication that comes from fixes in 
> templates (multiple instantiations create `const`-fix at the same position. 
> but because the warning message contains different type names,  they are not 
> deduplicated)
> - go through the review again and check if there are missing comments to 
> address
> - improve the documentation and give some hints on possible issues

These all seem to handle the fix-it portion. Would it be possible to split that 
into a follow-up review?

I am using tidy from CI and as a 'live-linter' from [n]vim. If I get a warning, 
it's easy to go to the line and type the six characters. In fact I don't even 
know the keystroke sequence that would instruct the editor to apply a fix-it :)

Obviously fix-its are not helping in CI, either - what is important is 
reporting at least one location and then the review gets rejected and 
subsequently revised.

I'm not saying I won't ever use the fix-its, but for now detection and 
reporting cover 100% of my use cases.

Thank you again for getting the energy and the time to work on this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 364997.
0x8000- added a comment.

Added separate check for parameter names.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s readability-variable-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-variable-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept
+{
+   int val;
+};
+
+struct simpleexcept
+{
+   int other;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: parameter name 'z' is too short, expected at least 3 characters [readability-variable-length]
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-variable-length]
+
+   int jj = z;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-variable-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const simpleexcept& e) // ignored via default configuration
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+
+   int x = 5;  // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
@@ -0,0 +1,76 @@
+.. title:: clang-tidy - readability-variable-length
+
+readability-variable-length
+===
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumExceptionNameLength`
+
+.. option:: MinimumLoopCounterNameLength
+
+Loop counter variables are expected to have a length of at least
+`MinimumLoopCounterNameLength` characters (default is `2`).
+
+.. code-block:: c++
+
+  // This warns that 'q' is too short.
+  for (int q = 0; q < size; ++ q) {
+ // ...
+  }
+
+.. option:: IgnoredLoopCounterNames
+
+Specifies a regular expression for counter names that are to be ignored.
+The default value is `^[ijk_]$`; the first three symbols for historical
+reasons and the last one since it is frequently used as a "dont't care"
+value, specifically in tools such as Google Benchmark.
+
+.. code-block:: c++
+
+  // This does not warn by default, for historical reasons.
+  for (int i = 0; i < size; ++ i) {
+  // ...
+  }
+
+.. option:: MinimumExceptionNameLength
+
+Exception clause variables are expected to have a length of at least
+`MinimumExceptionNameLength` (default is `2`).
+
+.. code-block:: c++
+
+  try {
+  // ...
+  }
+  // This warns that 'e' is too short.
+  catch (const std::exception& e) {
+  // ...
+  }
+
+.. option:: MinimumVariableNameLength
+
+All other variables are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`).
+
+.. code-block:: c++
+
+  int i = 42;// warns that 'i' is too 

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added a comment.

Added regex for exception names and rebased onto current 'main' branch.




Comment at: 
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+CheckFactories.registerCheck(
+"readability-variable-length");
   }

aaron.ballman wrote:
> Is there a reason this should be restricted to variables? For example, 
> wouldn't the same functionality be useful for type names, or dare I say it, 
> even macro names? I'm wondering if this should be 
> `readability-identifier-length` to be more generic.
I consider those to be in separate namespaces. I suppose we could have a single 
checker with multiple rules, but on the other hand I don't want to combine too 
many things into one, just because they share the "compare length" dimension.



Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;

aaron.ballman wrote:
> Should it be possible to enforce parameters differently than local variables? 
> (It seems a bit odd that we'd allow you to specify exception variable length 
> separate from loops but not function parameters separate from locals.)
Exceptions and Loops historically are "throw-away" / "don't care" / "one-off" 
variables. Parameter names are names, and they are the interface between the 
outside of the routine and the processing inside; other than historical, I 
don't see good arguments (sic) to allow single-letter parameter names.

Note that this check will be quite noisy on a legacy code base, and people will 
find little reason to invest the work to remove the warnings. But if somebody 
starts something new and want to enforce some quality attributes, it is the 
right tool at the right time. There will be new projects starting in 2021 and 
2022 that could benefit from this, I hope.



Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:26
+
+const char ErrorMessage[] = "%select{|exception |loop }0 variable name %1 is "
+"too short, expected at least %2 characters";

aaron.ballman wrote:
> It looks like there will be whitespace issues with this. If the variable is a 
> loop or exception, it'll have an extra space (`loop  variable name`) and if 
> it's not, it will start with a leading space (` variable name`).
This was recommended by a previous reviewer. Any alternative suggestion here?


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

https://reviews.llvm.org/D97753

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Ping?


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

https://reviews.llvm.org/D97753

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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D69764#2858656 , @MyDeveloperDay 
wrote:

> Rebase on clang12 as requested

Thank you!


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

https://reviews.llvm.org/D69764

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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Is it possible to have this rebased on top of LLVM12 for those of us who find 
the trade-offs acceptable? I prefer a consistent formatting of const placement, 
and my code base does not have issues with the macros - if we find one that 
break the build, I'll suggest it here as a test case ;)


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

https://reviews.llvm.org/D69764

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 327655.
0x8000- added a comment.

Add an option "IgnoredVariableNames" that allows filtering out acceptable 
variable names (similar to the loop counters); also implemented via regex.

Added tests for the IgnoredVariableNames option.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s readability-variable-length %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-variable-length.IgnoredVariableNames, value: "^[xy]$"}]}' \
+// RUN: --
+
+struct myexcept
+{
+   int val;
+};
+
+void doIt();
+
+void tooShortVariableNames(int z)
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: variable name 'z' is too short, expected at least 3 characters [readability-variable-length]
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-variable-length]
+
+   int jj = z;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-variable-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames(int y) // argument 'y' ignored by configuration
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+
+   int x = 5;  // ignored by configuration
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
@@ -0,0 +1,76 @@
+.. title:: clang-tidy - readability-variable-length
+
+readability-variable-length
+===
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumVariableNameLength`, :option:`IgnoredVariableNames`
+ - :option:`MinimumExceptionNameLength`
+
+.. option:: MinimumLoopCounterNameLength
+
+Loop counter variables are expected to have a length of at least
+`MinimumLoopCounterNameLength` characters (default is `2`).
+
+.. code-block:: c++
+
+  // This warns that 'q' is too short.
+  for (int q = 0; q < size; ++ q) {
+ // ...
+  }
+
+.. option:: IgnoredLoopCounterNames
+
+Specifies a regular expression for counter names that are to be ignored.
+The default value is `^[ijk_]$`; the first three symbols for historical
+reasons and the last one since it is frequently used as a "dont't care"
+value, specifically in tools such as Google Benchmark.
+
+.. code-block:: c++
+
+  // This does not warn by default, for historical reasons.
+  for (int i = 0; i < size; ++ i) {
+  // ...
+  }
+
+.. option:: MinimumExceptionNameLength
+
+Exception clause variables are expected to have a length of at least
+`MinimumExceptionNameLength` (default is `2`).
+
+.. code-block:: c++
+
+  try {
+  // ...
+  }
+  // This warns that 'e' is too short.
+  catch (const std::exception& e) {
+  // ...
+  }
+
+.. option:: MinimumVariableNameLength
+
+All other variables are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`).
+
+.. code-block:: c++
+
+  int i = 42;// warns 

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 327653.
0x8000- marked an inline comment as done.
0x8000- added a comment.

Use regular expression for filtering out loop counter variables to ignore.

Fully document the options.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s readability-variable-length %t
+
+struct myexcept
+{
+   int val;
+};
+
+void doIt();
+
+void tooShortVariableNames()
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-variable-length]
+
+   int jj = 6;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-variable-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames()
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
@@ -0,0 +1,70 @@
+.. title:: clang-tidy - readability-variable-length
+
+readability-variable-length
+===
+
+This check finds variables and function parameters whose length are too short.
+The desired name length is configurable.
+
+Special cases are supported for loop counters and for exception variable names.
+
+Options
+---
+
+The following options are described below:
+
+ - :option:`MinimumLoopCounterNameLength`, :option:`IgnoredLoopCounterNames`
+ - :option:`MinimumVariableNameLength`, :option:`MinimumExceptionNameLength`
+
+.. option:: MinimumLoopCounterNameLength
+
+Loop counter variables are expected to have a length of at least
+`MinimumLoopCounterNameLength` characters (default is `2`).
+
+.. code-block:: c++
+
+// This warns that 'q' is too short.
+for (int q = 0; q < size; ++ q) {
+// ...
+}
+
+.. option:: IgnoredLoopCounterNames
+
+Specifies a regular expression for counter names that are to be ignored.
+The default value is `^[ijk_]$`; the first three symbols for historical
+reasons and the last one since it is frequently used as a "dont't care"
+value, specifically in tools such as Google Benchmark.
+
+.. code-block:: c++
+
+// This does not warn by default, for historical reasons.
+for (int i = 0; i < size; ++ i) {
+// ...
+}
+
+.. option:: MinimumExceptionNameLength
+
+Exception clause variables are expected to have a length of at least
+`MinimumExceptionNameLength` (default is `2`).
+
+.. code-block:: c++
+
+try {
+// ...
+}
+// This warns that 'e' is too short.
+catch (const std::exception& e) {
+// ...
+}
+
+.. option:: MinimumVariableNameLength
+
+All other variables are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`).
+
+.. code-block:: c++
+
+int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
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
@@ -312,6 +312,7 @@

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst:9
+
+Loop counter variables are expected to have a length of at least
+`MinimumLoopCounterNameLength` characters (default is 2). Additionally, `i`,

Eugene.Zelenko wrote:
> Eugene.Zelenko wrote:
> > See other checks documentation as example of options descriptions.
> You still need to add `Options` section and mark-up for options. See other 
> checks documentation as example.
Oh, sorry, now I see what you mean. I have sampled randomly a handful of 
documentation snippets, and didn't find anything. But after your comment, I 
looked at readability-identifier-naming.rst which had "options" in all their 
glory.


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

https://reviews.llvm.org/D97753

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

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 327649.
0x8000- added a comment.

Addressed most of the review comments with the exception of switching to regex 
for ignoring loop counters; this change is in the follow-up review.


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

https://reviews.llvm.org/D97753

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp
  clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s readability-variable-length %t
+
+struct myexcept
+{
+   int val;
+};
+
+void doIt();
+
+void tooShortVariableNames()
+{
+   int i = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'i' is too short, expected at least 3 characters [readability-variable-length]
+
+   int jj = 6;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable name 'jj' is too short, expected at least 3 characters [readability-variable-length]
+
+   for (int m = 0; m < 5; ++ m)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: loop variable name 'm' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& x)
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: exception variable name 'x' is too short, expected at least 2 characters [readability-variable-length]
+   {
+  doIt();
+   }
+}
+
+void longEnoughVariableNames()
+{
+   int var = 5;
+
+   for (int i = 0; i < 42; ++ i) // 'i' is default allowed, for historical reasons
+   {
+  doIt();
+   }
+
+   for (int kk = 0; kk < 42; ++ kk)
+   {
+  doIt();
+   }
+
+   try
+   {
+  doIt();
+   }
+   catch (const myexcept& ex)
+   {
+  doIt();
+   }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-variable-length.rst
@@ -0,0 +1,45 @@
+.. title:: clang-tidy - readability-variable-length
+
+readability-variable-length
+===
+
+This check finds variables and function parameters whose length are too short.
+
+Loop counter variables are expected to have a length of at least
+`MinimumLoopCounterNameLength` characters (default is `2`). Additionally, `i`,
+`j`, `k` and `_` are accepted as legacy values (this is the default value for
+`IgnoredLoopCounterNames`).
+
+.. code-block:: c++
+
+   // This warns that 'q' is too short.
+   for (int q = 0; q < size; ++ q) {
+  /* */
+   }
+
+   // This does not warn, for historical reasons.
+   for (int i = 0; i < size; ++ i) {
+  /* */
+   }
+
+Exception clause variables are expected to have a length of at least
+`MinimumExceptionNameLength` (default is `2`).
+
+.. code-block:: c++
+
+   try {
+  /* */
+   }
+   // This warns that 'e' is too short.
+   catch (const std::exception& e) {
+   }
+
+All other variables are expected to have at least a length of
+`MinimumVariableNameLength` (default is `3`).
+
+.. code-block:: c++
+
+   int i = 42;// warns that 'i' is too short
+
+This check does not have any fix suggestions in the general case since
+variable names have semantic value.
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
@@ -312,6 +312,7 @@
`readability-uniqueptr-delete-release `_, "Yes"
`readability-uppercase-literal-suffix `_, "Yes"
`readability-use-anyofallof `_,
+   `readability-variable-length `_,
`zircon-temporary-objects `_,
 
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -89,6 +89,11 @@
   Finds member initializations in the constructor body which can be placed into
   the initialization list instead.
 
+- New :doc:`readability-variable-length
+  ` check.
+
+  Finds variables and function arguments whose names are too short.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h
===
--- /dev/null
+++ 

[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked 10 inline comments as done.
0x8000- added a comment.

@Eugene.Zelenko and @njames93

Thank you both for the kind and thoughtful feedback. I am incorporating all 
recommended changes.

First step are all the smaller scope comments, with regex support in a 
follow-up commit.

I will also generate the diffs in the future using the -U9 option as 
suggested.




Comment at: 
clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:58-62
+  Finder->addMatcher(
+  varDecl(unless(anyOf(hasParent(declStmt(hasParent(forStmt(,
+   hasParent(cxxCatchStmt()
+  .bind("standaloneVar"),
+  this);

njames93 wrote:
> This will miss the match for `k` in the same example:
> ```lang=c
> for (int i = 4, j = 5;;)
>   int k = 5;
> ```
> Gotta scratch my head for this one though. Though like the case before its 
> rather unlikely to show up in code so not the end of the world.
> 
> Also varDecl will match parameters, is this intended can you add tests and 
> explain in documentation.
> If its unintended putting `unless(parmVarDecl())` in the matcher disable that.
Matching of function parameter names is intentional. I will make sure it is 
emphasized in the documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97753

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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D69764#2532952 , @sammccall wrote:

> In D69764#2532666 , @MyDeveloperDay 
> wrote:
>
>>> What can be done to move this change along?
>>
>> I feel there has to be a fundamental acceptance that it is ok for 
>> clang-format to alter code (something it already does with sorting of 
>> includes, namespace comments).
>>
>> There were fairly strong opinions that clang-format isn't the best tool to 
>> do this (which actually I don't agree with, I think it is, as long as those 
>> capabilities are off by default and there is an acceptance they won't be 
>> perfect especially in the presence of macros due to lack of AST)
>>
>> My only thought about building another tool would be if it was a drop in 
>> replacement for clang-format (tooling allows setting of a path), but it 
>> would need to inherit all of clang-format.
>> but to me, this just feels like extra grunt work just to work around why 
>> some community don't like it.
>
> Yeah, this seems like adding a flag with extra steps.
>
> clang-format's brand is:
>
> - fast
> - semantic no-op
> - applies a consistent, project-specific style
>
> I think putting it (permanently) behind a flag or alternate binary would cut 
> against #3. I don't like that.

It can naturally be behind a flag, with is: east, west, leave_it_be. Skittish 
people/teams can leave_it_be for a release or two.

> If it's buggy, this feature risks cutting against #2 (more than usual). So 
> code supporting this feature is more critical than it was previously, and 
> that might be a lot of heuristics.

How is the risk of this bugginess greater than the risk of any other 
transformation? (header sorting can expose invalid dependencies - and break the 
build).


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

https://reviews.llvm.org/D69764

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Would it be possible to split this review further, into "checking" and "fixing" 
portions? I understand that fix-it portion is more difficult, and sometimes 
results in multiple 'const' keywords being added, but for the past year we have 
used the check as part of regular CI runs to flag where it needs to be added by 
the engineers and for our use case it works fine that way - so I would prefer 
to have at least the "report" part as part of upstream Clang12, even if the 
'fixup' comes later, due to the complexity of covering all corner cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2292377 , @0x8000- wrote:

> In D54943#2291969 , @JonasToth wrote:
>
>> @AlexanderLanin @0x8000- i created the branch `release-11-const` 
>> (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.
>>
>> This branch is based on 11-rc3 and cherry picks the commits that correspond 
>> to this revision.
>> I hope this is of any use for you! I will probably forget to update this 
>> branch all the time, but if I feel like there has been actual progress made, 
>> i will update!
>
> Thank you Jonas - will add this to our CI tool for the new project and try to 
> find some time to run it on the legacy project.

Just a quick update; in clang-tidy mode it works great, with no false positive 
and no crashes. I haven't tried the 'apply-replacements' mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2291969 , @JonasToth wrote:

> @AlexanderLanin @0x8000- i created the branch `release-11-const` 
> (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.
>
> This branch is based on 11-rc3 and cherry picks the commits that correspond 
> to this revision.
> I hope this is of any use for you! I will probably forget to update this 
> branch all the time, but if I feel like there has been actual progress made, 
> i will update!

Thank you Jonas - will add this to our CI tool for the new project and try to 
find some time to run it on the legacy project.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2290713 , @AlexanderLanin 
wrote:

>> Could you please provide me a full reproducer (optimally without 
>> dependencies on includes/libraries)?
>
> I can certainly do that based on my old version from ~9 months ago or 
> preferably if you provide me some branch somewhere (github?). I have trouble 
> applying the latest diff to latest master myself.

https://github.com/JonasToth/llvm-project/tree/feature_const_transform is close 
to the origin/master (a few commits behind).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2289410 , @JonasToth wrote:

> In D54943#2289037 , @0x8000- 
> wrote:
>
>> Master branch has too many false positives for tidy - would it be possible 
>> to create a branch that contains this patch set on top of llvm-11.0-rc3? I 
>> would then add this to our internal CI.
>
> Yeah, it is not that simple to get this check into something really usefull :/

Just to be clear, the false positives I was referring to are not in this 
checker, but other checkers (uninitialized variables mostly) that were 
committed to the origin/main branch.

> I can create a branch in my own fork, that would need a little work, which i 
> can do in ~8 hours. I would then ping you.

Thank you - I tried cherry-picking the changes from the main branch onto the 
release branch and ... it does not build - didn't have time to debug it - and I 
am hoping you're more quick at making the adaptation.

>> For the legacy code (see previous reports) we found quite a few false 
>> positives. For a new code base that we're developing from scratch with 
>> C++20, there have been no false positives - it catches missed 'const' much 
>> better than human reviewers - we still keep a Clang10 around that I built 
>> from source with the previous version of the patches :)
>
> It would be great if you could keep testing your legacy code-base once this 
> patch progresses even further.
> It is a great message, that the modern codebase does not make big trouble :) 
> Given I tested mostly on LLVM itself so far, this is not too surprising as 
> its a modern codebase itself.

I will keep testing on both codebases - the modern one we're using it in the CI 
so it gets tested vigorously. Once I get some time I'll take another pass at 
the legacy code base to see what it finds.

Thank you for resuming the work on this. I was afraid I'm going to lose this 
check when we have to move off Clang10 - (we're pursuing C++20 aggressively in 
the new project).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Master branch has too many false positives for tidy - would it be possible to 
create a branch that contains this patch set on top of llvm-11.0-rc3? I would 
then add this to our internal CI.

For the legacy code (see previous reports) we found quite a few false 
positives. For a new code base that we're developing from scratch with C++20, 
there have been no false positives - it catches missed 'const' much better than 
human reviewers - we still keep a Clang10 around that I built from source with 
the previous version of the patches :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

@MyDeveloperDay +1 from the trenches - I am in that same position and it took a 
lot of work to get the organization to _align_ on a consistent style, then 
enforce that.


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

https://reviews.llvm.org/D69764



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1815916 , @Eugene.Zelenko 
wrote:

> In D54943#1815912 , @0x8000- 
> wrote:
>
> > As an aside, once this is merged in, I dream of a "fix-it" for old style C 
> > code:
> >
> >   int foo;
> >  
> >   ...
> >   /* page of code which does not use either foo or bar */
> >   ...
> >   foo = 5;
> >
> >
> > Moves the foo declaration to the line where it is actually first 
> > initialized. Then run the constify tool on top.
>
>
> You are not first :-)  See PR21983.


There's https://reviews.llvm.org/D64671 which solves half the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

As an aside, once this is merged in, I dream of a "fix-it" for old style C code:

  int foo;
  
  ...
  /* page of code which does not use either foo or bar */
  ...
  foo = 5;

Moves the foo declaration to the line where it is actually first initialized. 
Then run the constify tool on top.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

One more mis-constification that I can't explain, nor reduce to a small test 
case (when I extract the code, the check behaves as expected / desired)

  namespace util {
struct StringIgnoreInitialHash : public std::unary_function
{
   size_t operator()(const std::string& rhs) const
   {
  std::size_t hash = 0;
  std::string::const_iterator str = rhs.begin();
  std::string::const_iterator end = rhs.end();
  if (str != end)
  {
 boost::hash_combine(hash, std::toupper(*str, std::locale()));
 ++str;
  }
  for (; str != end; ++str)
  {
 boost::hash_combine(hash, *str);
  }
  return hash;
   }
};
  }

This is in a header included 29 times in other headers and 106 times directly 
in translation units.

The const-checker reported 31 times that the variable 'hash' can be made 
constant.

Also it reported 5 times that the variable 'str' can be made constant (which it 
can't) and 194 times that variable 'end' can be made constant (which it can, 
and should).

Running in fix mode, made 'hash', 'str' and 'end' all constants.

Extracting this into its own translation unit and adding the requisite 
\#includes and paths to the boost files does not reproduce the error. Only 
'end' is, correctly, reported as needing to be const.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Summary of "misses"

One:

  uint32_t counter = 0;
  BOOST_FOREACH(const Foo* foo, *theFoos)
  {
  if (foo->getType() == someType)
  {
  ++counter;
  }
  }

The -fix makes the counter const :), so obviously it breaks compilation.

Two:

It marks a lot of std::stringstream / std::ostringstream instances as const. So 
we can't << into it, breaking compilation.

Three:

It marked an array in the header as const. There were some pointer variables 
(in a translation unit which included that header) that were declared and 
initialized to 0 (meaning nullptr), then later in a case block of a switch they 
were assigned [0] value. It did not change the pointer variables, but 
they used to be mutable pointer to mutable, so now the assignment from a const 
array failed to build.

The changes required to get the build complete (7500 translation units);

  15 files changed, 59 insertions(+), 59 deletions(-)

I can live with the breakage in the 3rd case - since the fix is one time and 
straightforward (and makes the code clearly better). But I prefer not to have 
to annotate all std::sotringstream instances with NOLINT. Also we have a fair 
number of BOOST_FOREACH macros from way back then. They should be ripped out of 
course, but it's in code that wasn't touched in a while, so having the 
clang-tidy keep reporting / misfixing that area is bothersome.

I really want this clang-tidy tool, but I don't want to put off users via false 
positives. Many don't want to deal with it anyway, and the smallest excuse will 
be brought up against enabling the use of the tool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

I have built diff14 and tried to apply it on an internal code base with ~7500 
translation units.

  $ python3 /opt/clang10/share/clang/run-clang-tidy.py -clang-tidy-binary  
/opt/clang10/bin/clang-tidy -clang-apply-replacements 
/opt/clang10/bin/clang-apply-replacements -p ../build.clang10.dbg/ 
"-checks=-*,cppcoreguidelines-const-correctness"  -fix -j 24 -quiet 
-deduplicate $(find lib -name \*.cpp) > const_fix.log 2&> const_err.log

The diffstat is

  1608 files changed, 19927 insertions(+), 19927 deletions(-)

This generated 56 "const const" declarations, of which 25 were triple const!

I have tried -deduplicate argument as in Jonas' script, but ...

  run-clang-tidy.py: error: unrecognized arguments: -deduplicate

Now onto editing those files by hand to remove the duplications and re-running 
the build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1815568 , @JonasToth wrote:

> @0x8000- i did remove `dependentTypes` from matching. Locally that works 
> with your reported test-case (i was running clang-tidy wrong :().
>
> I do understand the `auto &` issues now (i think).
>  If the type is deduced to a template-type or something that depends the 
> standard ways of detecting that do not work, because in each template 
> instantiation this information is not available.
>
> Having a `auto & local = TemplatedVariable` does not have the 
> `isInstantiationDependentType()`-bits set and for each instantiation of the 
> template the const-analysis is run.
>  The false positives happened in different template instantiations, i think 
> because the overloaded operators were sometimes `const` and sometimes not. I 
> tried many ways to detect, if the type comes from a template, but had no 
> success.
>  This is probably an issue in `Sema` or requires adjustments there.
>  I added tests in clang-tidy, but `#if 0` them out because are not working 
> right now.


Thank you for looking into this; I'll start building diff14 to test locally.

I'm good with merging this checker as-is, as long as it is not enabled by 
default. I want to be able to use it even partially, in environments where I 
might not be able to inject a home-built compiler ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1815473 , @JonasToth wrote:

> In D54943#1815358 , @0x8000- 
> wrote:
>
> > Here is a minimal example that shows the problem:
>
>
> I can not reproduce that case :(
>  It gives me a compilation error in `const DoGooder anInstanceOf...`, because 
> `DoGooder` requires an template parameter. I then add `DoGooder` and the 
> variable gets no transformation.
>  Did you reproduce the error with exactly that code?
>
> And which version did you run? Maybe that was a previous false positive, that 
> might be fixed right now?


The way I am testing is that I am fetching the current master branch of 
https://github.com/llvm/llvm-project.git, then downloading the latest patch 
from here and applying on top, then building.

This is the state of the tree right as of last night:

  commit 1c8ab48028cc39429a392691ef2e66968460d782 (HEAD -> D54943.diff12)
  Author: Florin Iucha 
  Date:   Fri Jan 10 18:52:54 2020 -0500
  
  D54943.diff12
  
  commit 1b8c84b8dd5a4a294943a6a6f0631d2d3a1f9f27 (origin/master, origin/HEAD, 
master)
  Author: Richard Smith 
  Date:   Fri Jan 10 15:47:29 2020 -0800
  
  Improve precision of documentation comment.

CTAD takes care of the missing template parameter, that's why I'm passing 
"-std=c++17" to clang-tidy.

  /tmp$ /opt/clang10/bin/clang-tidy 
-checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" test.cpp 
-- -std=c++17 -Wall
  54 warnings generated.
  /tmp/test.cpp:22:9: warning: variable 'anInstanceOf' of type '' can be declared 'const' [cppcoreguidelines-const-correctness]
  const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
  ^
 const 
  Suppressed 53 warnings (53 in non-user code).
  Use -header-filter=.* to display errors from all non-system headers. Use 
-system-headers to display errors from system headers as well.
  /tmp$ /opt/clang10/bin/clang++ -c test.cpp
  test.cpp:22:15: error: use of class template 'DoGooder' requires template 
arguments
  const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
^
  test.cpp:4:8: note: template is declared here
  struct DoGooder
 ^
  1 error generated.
  /tmp$ /opt/clang10/bin/clang++ -std=c++17 -c test.cpp
  -rw-r--r-- 1 florin florin 432 Jan 11 09:48 test.cpp
  -rw-r--r-- 1 florin florin 744 Jan 11 09:52 test.o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Here is a minimal example that shows the problem:

  #include 
  
  template
  struct DoGooder
  {
  DoGooder(void* accessor, SomeValue value)
  {
  }
  
  };
  
  struct Bingus
  {
  static constexpr auto someRandomConstant = 99;
  };
  
  template
  struct HardWorker
  {
  HardWorker()
  {
  const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
  }
  };
  
  struct TheContainer
  {
  std::unique_ptr> m_theOtherInstance;
  };

Example run:

  $ /opt/clang10/bin/clang-tidy 
-checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" 
reproconst.cpp -- -std=c++17 -Wall
  54 warnings generated.
  reproconst.cpp:22:9: warning: variable 'anInstanceOf' of type '' can be declared 'const' [cppcoreguidelines-const-correctness]
  const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
  ^
 const
  Suppressed 53 warnings (53 in non-user code).
  Use -header-filter=.* to display errors from all non-system headers. Use 
-system-headers to display errors from system headers as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1815121 , @JonasToth wrote:

> In D54943#1815073 , @0x8000- 
> wrote:
>
> > Applied the fix-it on one of our smaller (but C++ code bases) and it builds 
> > and passes the tests. Comments:
> >
> > 1. I'm still seeing some false positives, where some locals that are const 
> > are flagged as could be made const - I'm working to create a minimal 
> > reproduction recipe.
> > 2. We're a west const shop :) so adding a bunch of east-consts will not fly 
> > well. Is there a way to configure where 'const' is placed by the fixit? 
> > (Specifically 'auto const', we prefer it 'const auto').
>
>
> Does it build now? I couldn't find a way to reproduce that and gave up, tbh.
>
> 1. Template context? Auto involved? I saw some double-analysis for `auto&`, 
> because clang-tidy didn't ignore those properly. And are you using 
> `run-clang-tidy`? It deduplicates fixits, maybe that is involved?
> 2. YesNo, The utility for adding const is able to do both, west const has 
> problems with `typedef int * MyType;` scenarios, where the `const` will apply 
> to the wrong thing. Doing that right requires special care. BUT: 
> `clang-format` has a east-const/west-const feature now (i think new with this 
> release). So I am now somewhat considering to let clang-format do that for me.
>
>   Thanks again for taking a look at it. But if the issues you found are new, 
> i think we should maybe not commit this weekend.


I haven't tried Debug build, Release only.

I'm good with having clang-format do the west-const transformation.

It's not a template, but something like "const Foo foo{x, y, z};" where x and y 
were references and z was a pointer. I'll try to reduce a test case.

I have also noticed a check failure, but not sure if in this tool or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Applied the fix-it on one of our smaller (but C++ code bases) and it builds and 
passes the tests. Comments:

1. I'm still seeing some false positives, where some locals that are const are 
flagged as could be made const - I'm working to create a minimal reproduction 
recipe.
2. We're a west const shop :) so adding a bunch of east-consts will not fly 
well. Is there a way to configure where 'const' is placed by the fixit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1805439 , @JonasToth wrote:

> > This last version too builds with RelWithDebInfo and fails with Debug 
> > configuration (both clean builds).
>
> So it only fails in debug-build?


Correct; I can build 'Release' or 'RelWithDebInfo' but not 'Debug'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1804943 , @JonasToth wrote:

> - Merge branch 'master' into feature_transform_const.patch
> - link clangAnalysis to the cppcoreguidelines-module
> - fix InitListExpr as well


This last version too builds with RelWithDebInfo and fails with Debug 
configuration (both clean builds).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1804904 , @JonasToth wrote:

> In D54943#1804890 , @0x8000- 
> wrote:
>
> > I can't build this on top of current llvm-project tree 
> > (6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7 
> > ):
> >
> >   /usr/bin/ld: 
> > lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in 
> > function 
> > `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
> >   
> > /home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65:
> >  undefined reference to `clang::ast_matchers::decompositionDecl'
> >
> >
> > I am building using clang++-9 from Debian, and diff7 applied cleanly.
> >
> > Jonas, is it possible to rebase this on top of llvm-project and push it to 
> > a branch on https://github.com/JonasToth/llvm-project ?
>
>
> I just merged master into the branch and pushed to 
> `feature_transform_const.patch` branch in my fork. Maybe that works?
>  If you want you can join IRC, its faster to get this fixed there :)


Strange - I have checked out your branch, and the patch is correct (no 
difference between when I apply the phabricator diff7 on top of upstream 
llvm-project and when i look at feature_transform_const.patch)

I am using the following cmake configuration:

  cmake ../llvm \
 -DCMAKE_INSTALL_PREFIX=/opt/clang10 \
 -DCMAKE_C_COMPILER=/usr/bin/clang-9 \
 -DCMAKE_CXX_COMPILER=/usr/bin/clang++-9 \
 -DCMAKE_BUILD_TYPE=Debug \
 -DLLVM_TARGETS_TO_BUILD=X86 \
 -DLLVM_ENABLE_THREADS=On \
 -DLLVM_INSTALL_TOOLCHAIN_ONLY=On \
 -DLLVM_USE_SPLIT_DWARF=On \
 -DLLVM_BUILD_TESTS=ON \
 
-DLLVM_ENABLE_PROJECTS="clang;libcxx;libcxxabi;libunwind;lldb;compiler-rt;lld;clang-tools-extra;"
 \
 -GNinja

What cmake configuration are you using?

(Sorry - I need to go, no time for IRC today)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

I can't build this on top of current llvm-project tree 
(6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7 
):

  /usr/bin/ld: 
lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in 
function 
`clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
  
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65:
 undefined reference to `clang::ast_matchers::decompositionDecl'

I am building using clang++-9 from Debian, and diff7 applied cleanly.

Jonas, is it possible to rebase this on top of llvm-project and push it to a 
branch on https://github.com/JonasToth/llvm-project ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template 

JonasToth wrote:
> JonasToth wrote:
> > 0x8000- wrote:
> > > JonasToth wrote:
> > > > 0x8000- wrote:
> > > > > 0x8000- wrote:
> > > > > > JonasToth wrote:
> > > > > > > JonasToth wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > Please insert the this test code here:
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > struct IntWrapper {
> > > > > > > > > 
> > > > > > > > >unsigned low;
> > > > > > > > >unsigned high;
> > > > > > > > > 
> > > > > > > > >IntWrapper& operator=(unsigned value) {
> > > > > > > > >   low = value & 0x;
> > > > > > > > >   high = (value >> 16) & 0x;
> > > > > > > > >}
> > > > > > > > > 
> > > > > > > > >template
> > > > > > > > >friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
> > > > > > > > >   unsigned someValue = 0;   // false positive now
> > > > > > > > >   if (is >> someValue) {
> > > > > > > > >  rhs = someValue;
> > > > > > > > >   }
> > > > > > > > >   return is;
> > > > > > > > >}
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > unsigned TestHiddenFriend(IntMaker& im) {
> > > > > > > > >IntWrapper iw;
> > > > > > > > > 
> > > > > > > > >im >> iw;
> > > > > > > > > 
> > > > > > > > >return iw.low;
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > clang gives me no error when I add the const there. sure it 
> > > > > > > > does reproduce properly?
> > > > > > > Here for reference: https://godbolt.org/z/DXKMYh
> > > > > > Probably I wasn't clear - I suggested adding my test code at line 
> > > > > > 608, because it needs the "IntMaker" definition at line 594.
> > > > > > 
> > > > > > The false positive from this const check is reported on the 
> > > > > > "unsigned someValue = 0;" line inside the template extraction 
> > > > > > operator.
> > > > > Oh, I got it - don't think "shift" think "overloaded extraction 
> > > > > operator".
> > > > > 
> > > > > In my code above, you don't know what ">>" does, and it clearly takes 
> > > > > a mutable reference as the right side.
> > > > > 
> > > > > ```
> > > > > const int foo;
> > > > > std::cin >> foo;
> > > > > ```
> > > > > 
> > > > > should not compile, either :)
> > > > no. something else is going on.
> > > > https://godbolt.org/z/xm8eVC
> > > > ```
> > > > | |   |-CXXOperatorCallExpr  ''
> > > > | |   | |-UnresolvedLookupExpr  '' 
> > > > lvalue (ADL) = 'operator>>' 0x55a26b885938 0x55a26b857238
> > > > | |   | |-DeclRefExpr  'Istream' lvalue ParmVar 0x55a26b885748 
> > > > 'is' 'Istream &'
> > > > | |   | `-DeclRefExpr  'const unsigned int' lvalue Var 
> > > > 0x55a26b885a38 'someValue' 'const unsigned int'
> > > > ```
> > > > This code is only a false positive in the sense, that the "we can not 
> > > > know if something bad happens" is not detected. The operator>> is not 
> > > > resolved. That is because the template is not instantiated and the 
> > > > expressions can therefore not be resolved yet.
> > > > There seems to be no instantiation of this template function.
> > > > 
> > > > Somehow the `im >> iw;` does not instantiate the `friend operator<<`. I 
> > > > reduced it to something i think is minimal and that shows the same 
> > > > behaviour. (https://godbolt.org/z/MMG_4q)
> > > https://godbolt.org/z/7QEdHJ
> > > 
> > > ```
> > > struct IntMaker {
> > >   operator bool() const;
> > > 
> > >   friend IntMaker >>(IntMaker &, unsigned &);
> > >   //friend IntMaker >>(IntMaker &, const unsigned &) = delete;
> > > };
> > > ```
> > > 
> > > If you uncomment the deleted overload then
> > > 
> > > ```
> > > template 
> > > Istream& operator>>(Istream& is, IntWrapper& rhs)  {
> > > unsigned const someValue = 0;
> > > if (is >> someValue) {
> > > rhs = someValue;
> > > }
> > > return is;
> > > }
> > > ```
> > > 
> > > Fails to compile.
> > > 
> > > Depending on what else is around, it seems that somehow the compiler 
> > > would try to use the (bool) conversion to obtain an integral then use it 
> > > as an argument to the built-in integral left shift.
> > > 
> > > https://godbolt.org/z/-JFL5o - this does not compile, as expected:
> > > 
> > > ```
> > > #include 
> > > 
> > > int readInt() {
> > > const int foo = 0;
> > > std::cin >> foo;
> > > return foo;
> > > }
> > > ```
> > > 
> > > so this check should not recommend making foo constant.
> > I see. Implicit conversions are great... :)
> > 
> > I will recheck that. And the `std::cin` example is analyzed correctly. I 
> > added a test for that, too.
> > Thank you for researching the issue!
> https://godbolt.org/z/VerWce
> Minimal example that shows the issue.
As much as I would have liked to contribute a good test to your new checker, 
I'm happier that I have 

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template 

JonasToth wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > JonasToth wrote:
> > > > JonasToth wrote:
> > > > > 0x8000- wrote:
> > > > > > Please insert the this test code here:
> > > > > > 
> > > > > > ```
> > > > > > struct IntWrapper {
> > > > > > 
> > > > > >unsigned low;
> > > > > >unsigned high;
> > > > > > 
> > > > > >IntWrapper& operator=(unsigned value) {
> > > > > >   low = value & 0x;
> > > > > >   high = (value >> 16) & 0x;
> > > > > >}
> > > > > > 
> > > > > >template
> > > > > >friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
> > > > > >   unsigned someValue = 0;   // false positive now
> > > > > >   if (is >> someValue) {
> > > > > >  rhs = someValue;
> > > > > >   }
> > > > > >   return is;
> > > > > >}
> > > > > > };
> > > > > > 
> > > > > > unsigned TestHiddenFriend(IntMaker& im) {
> > > > > >IntWrapper iw;
> > > > > > 
> > > > > >im >> iw;
> > > > > > 
> > > > > >return iw.low;
> > > > > > }
> > > > > > ```
> > > > > clang gives me no error when I add the const there. sure it does 
> > > > > reproduce properly?
> > > > Here for reference: https://godbolt.org/z/DXKMYh
> > > Probably I wasn't clear - I suggested adding my test code at line 608, 
> > > because it needs the "IntMaker" definition at line 594.
> > > 
> > > The false positive from this const check is reported on the "unsigned 
> > > someValue = 0;" line inside the template extraction operator.
> > Oh, I got it - don't think "shift" think "overloaded extraction operator".
> > 
> > In my code above, you don't know what ">>" does, and it clearly takes a 
> > mutable reference as the right side.
> > 
> > ```
> > const int foo;
> > std::cin >> foo;
> > ```
> > 
> > should not compile, either :)
> no. something else is going on.
> https://godbolt.org/z/xm8eVC
> ```
> | |   |-CXXOperatorCallExpr  ''
> | |   | |-UnresolvedLookupExpr  '' lvalue 
> (ADL) = 'operator>>' 0x55a26b885938 0x55a26b857238
> | |   | |-DeclRefExpr  'Istream' lvalue ParmVar 0x55a26b885748 'is' 
> 'Istream &'
> | |   | `-DeclRefExpr  'const unsigned int' lvalue Var 0x55a26b885a38 
> 'someValue' 'const unsigned int'
> ```
> This code is only a false positive in the sense, that the "we can not know if 
> something bad happens" is not detected. The operator>> is not resolved. That 
> is because the template is not instantiated and the expressions can therefore 
> not be resolved yet.
> There seems to be no instantiation of this template function.
> 
> Somehow the `im >> iw;` does not instantiate the `friend operator<<`. I 
> reduced it to something i think is minimal and that shows the same behaviour. 
> (https://godbolt.org/z/MMG_4q)
https://godbolt.org/z/7QEdHJ

```
struct IntMaker {
  operator bool() const;

  friend IntMaker >>(IntMaker &, unsigned &);
  //friend IntMaker >>(IntMaker &, const unsigned &) = delete;
};
```

If you uncomment the deleted overload then

```
template 
Istream& operator>>(Istream& is, IntWrapper& rhs)  {
unsigned const someValue = 0;
if (is >> someValue) {
rhs = someValue;
}
return is;
}
```

Fails to compile.

Depending on what else is around, it seems that somehow the compiler would try 
to use the (bool) conversion to obtain an integral then use it as an argument 
to the built-in integral left shift.

https://godbolt.org/z/-JFL5o - this does not compile, as expected:

```
#include 

int readInt() {
const int foo = 0;
std::cin >> foo;
return foo;
}
```

so this check should not recommend making foo constant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1804291 , @JonasToth wrote:

> In D54943#1804183 , @0x8000- 
> wrote:
>
> > This patch does not build on top of current tree:
> >
> >   /usr/bin/ld: 
> > lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in 
> > function 
> > `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
> >   
> > /home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65:
> >  undefined reference to `clang::ast_matchers::decompositionDecl'
> >   clang: error: linker command failed with exit code 1 (use -v to see 
> > invocation)
> >   [2771/4997] Building CXX object 
> > tools/clang/tools/extra/clang-include-fixer/find-all-symbols/CMakeFiles/obj.findAllSymbols.dir/PragmaCommentHandler.cpp.o
> >   ninja: build stopped: subcommand failed.
> >
> >
> > This is my top of the tree right now:
> >
> >   b7ecf1c1c373c53183ef6ef66efbe4237ff7b96d (origin/master, origin/HEAD, 
> > master) NFC: Fix trivial typos in comments
> >
>
>
> did the changes to `clang/include/clang/ASTMatchers/ASTMatchers.h` and 
> `clang/lib/ASTMatchers/Dynamic/Registry.cpp` apply? They provide the 
> `decompositionDecl` matcher. It seems odd, they are not available :/


This is the result of applying the patch as e-mailed:

  ~/tools/llvm-project$ patch -p0 < /tmp/D54943.236101.patch


  patching file clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp   


  patching file clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp


  patching file clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp


  patching file clang/lib/Analysis/ExprMutationAnalyzer.cpp 


  patching file clang/lib/ASTMatchers/Dynamic/Registry.cpp  


  patching file clang/include/clang/ASTMatchers/ASTMatchers.h   


  patching file 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
   
  patching file 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
 
  patching file 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
  patching file 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp

  patching file 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp

  patching file 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst

  patching file clang-tools-extra/docs/ReleaseNotes.rst 


  patching file 
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp  

  patching file 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h  

  patching file 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

  patching file clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt   


  patching file clang-tools-extra/CMakeLists.txt

No rejection that I can see.

  ~/tools/llvm-project$ ag decompositionDecl
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  67: unless(has(decompositionDecl()

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

This patch does not build on top of current tree:

  /usr/bin/ld: 
lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in 
function 
`clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
  
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65:
 undefined reference to `clang::ast_matchers::decompositionDecl'
  clang: error: linker command failed with exit code 1 (use -v to see 
invocation)
  [2771/4997] Building CXX object 
tools/clang/tools/extra/clang-include-fixer/find-all-symbols/CMakeFiles/obj.findAllSymbols.dir/PragmaCommentHandler.cpp.o
  ninja: build stopped: subcommand failed.

This is my top of the tree right now:

  b7ecf1c1c373c53183ef6ef66efbe4237ff7b96d (origin/master, origin/HEAD, master) 
NFC: Fix trivial typos in comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template 

0x8000- wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > 0x8000- wrote:
> > > > Please insert the this test code here:
> > > > 
> > > > ```
> > > > struct IntWrapper {
> > > > 
> > > >unsigned low;
> > > >unsigned high;
> > > > 
> > > >IntWrapper& operator=(unsigned value) {
> > > >   low = value & 0x;
> > > >   high = (value >> 16) & 0x;
> > > >}
> > > > 
> > > >template
> > > >friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
> > > >   unsigned someValue = 0;   // false positive now
> > > >   if (is >> someValue) {
> > > >  rhs = someValue;
> > > >   }
> > > >   return is;
> > > >}
> > > > };
> > > > 
> > > > unsigned TestHiddenFriend(IntMaker& im) {
> > > >IntWrapper iw;
> > > > 
> > > >im >> iw;
> > > > 
> > > >return iw.low;
> > > > }
> > > > ```
> > > clang gives me no error when I add the const there. sure it does 
> > > reproduce properly?
> > Here for reference: https://godbolt.org/z/DXKMYh
> Probably I wasn't clear - I suggested adding my test code at line 608, 
> because it needs the "IntMaker" definition at line 594.
> 
> The false positive from this const check is reported on the "unsigned 
> someValue = 0;" line inside the template extraction operator.
Oh, I got it - don't think "shift" think "overloaded extraction operator".

In my code above, you don't know what ">>" does, and it clearly takes a mutable 
reference as the right side.

```
const int foo;
std::cin >> foo;
```

should not compile, either :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template 

JonasToth wrote:
> JonasToth wrote:
> > 0x8000- wrote:
> > > Please insert the this test code here:
> > > 
> > > ```
> > > struct IntWrapper {
> > > 
> > >unsigned low;
> > >unsigned high;
> > > 
> > >IntWrapper& operator=(unsigned value) {
> > >   low = value & 0x;
> > >   high = (value >> 16) & 0x;
> > >}
> > > 
> > >template
> > >friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
> > >   unsigned someValue = 0;   // false positive now
> > >   if (is >> someValue) {
> > >  rhs = someValue;
> > >   }
> > >   return is;
> > >}
> > > };
> > > 
> > > unsigned TestHiddenFriend(IntMaker& im) {
> > >IntWrapper iw;
> > > 
> > >im >> iw;
> > > 
> > >return iw.low;
> > > }
> > > ```
> > clang gives me no error when I add the const there. sure it does reproduce 
> > properly?
> Here for reference: https://godbolt.org/z/DXKMYh
Probably I wasn't clear - I suggested adding my test code at line 608, because 
it needs the "IntMaker" definition at line 594.

The false positive from this const check is reported on the "unsigned someValue 
= 0;" line inside the template extraction operator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Indicated where the new test code should go.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template 

Please insert the this test code here:

```
struct IntWrapper {

   unsigned low;
   unsigned high;

   IntWrapper& operator=(unsigned value) {
  low = value & 0x;
  high = (value >> 16) & 0x;
   }

   template
   friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
  unsigned someValue = 0;   // false positive now
  if (is >> someValue) {
 rhs = someValue;
  }
  return is;
   }
};

unsigned TestHiddenFriend(IntMaker& im) {
   IntWrapper iw;

   im >> iw;

   return iw.low;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#1803448 , @JonasToth wrote:

> In D54943#1800182 , @0x8000- 
> wrote:
>
> > F11163406: 0001-Add-extra-tests.patch  
> > shows a couple of false positives.
>
>
> I added your tests, but some did not show false positives anymore. I think i 
> will remove some, that i find redundant.
>  But could you please recheck with the current version first, if this is 
> actually correct and the original false positives are gone?


I can confirm that the false positives in the real code have been cleaned up. 
Thank you!

However I would be loath to discard tests - unless we can prove they are truly 
redundant.

Also, here is a new test for you - it trips now on "unsigned someValue = 0;" 
line:

  struct IntWrapper {
 unsigned low;
 unsigned high;
  
 IntWrapper& operator=(unsigned value) {
low = value & 0x;
high = (value >> 16) & 0x;
 }
  
 template
 friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
unsigned someValue = 0;
if (is >> someValue) {
   rhs = someValue;
}
return is;
 }
  };
  
  
  unsigned TestHiddenFriend(IntMaker& im) {
 IntWrapper iw;
  
 im >> iw;
  
 return iw.low;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

F11163406: 0001-Add-extra-tests.patch  
shows a couple of false positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

@aaron.ballman updated as suggested; please commit/integrate when you have a 
moment. Thank you!


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked 2 inline comments as done.
0x8000- added a comment.

In D71686#1794366 , @aaron.ballman 
wrote:

> In D71686#1794360 , @0x8000- 
> wrote:
>
> > In D71686#1794330 , @aaron.ballman 
> > wrote:
> >
> > > In D71686#1794053 , @0x8000- 
> > > wrote:
> > >
> > > > My take: this change fixes a user-reported bug, and does not cause any 
> > > > known regressions. I think we should integrate this.
> > >
> > >
> > > I sort of wonder whether we want to document this as a blessed approach 
> > > to silencing the warning. I'm not certain if it's too obtuse or not, but 
> > > I notice the check has no documented ways to silence the diagnostic aside 
> > > from using the correct kind of magic number or adding it to a list of 
> > > excluded magic numbers.
> >
> >
> > You mean Hyrum's Law  is not sufficient?
> >
> > The check can be silenced with the regular NOLINT, or with defining and 
> > using a constant/enum. Using this "backdoor" way seems even more cumbersome 
> > and confusing than the NOLINT. At least with NOLINT it is clear what you're 
> > doing, and somebody else can grep for it and fix it if it is appropriate.
>
>
> My concern is that `NOLINT` is insufficient. Consider: `foo(12, 42, 18);` 
> where the `42` is not desired to be warned about due to domain-specific 
> knowledge but the `12` and `18` are.
>
> However, I am not convinced that you're wrong either -- casting is 
> cumbersome. But it's also a somewhat well-used workaround to silence warnings 
> (casting to void silencing unused value warnings being a common example).
>
> For right now, we can leave it as a bug -- we can decide to bless the 
> approach later.


Fair enough; I'm aiming for good precision and recall by default.


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 235060.
0x8000- added a comment.

Minor comment fixes; capitalization, full stop.


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

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 
@@ -183,7 +183,7 @@
 
 const geometry::Rectangle mandelbrotCanvas{geometry::Point{-2.5, -1}, geometry::Dimension{3.5, 2}};
 
-// Simulate the macro magic in Google Test internal headers
+// Simulate the macro magic in Google Test internal headers.
 class AssertionHelper {
 public:
   AssertionHelper(const char *Message, int LineNumber) : Message(Message), LineNumber(LineNumber) {}
@@ -201,7 +201,7 @@
   ASSERTION_HELPER("here and now");
 }
 
-// prove that integer literals introduced by the compiler are accepted silently
+// Prove that integer literals introduced by the compiler are accepted silently.
 extern int ConsumeString(const char *Input);
 
 const char *SomeStrings[] = {"alpha", "beta", "gamma"};
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// Prove that using enumerations values don't produce warnings.
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t --
+// XFAIL: *
+
+int ProcessSomething(int input);
+
+int DoWork()
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+return 0;
+
+   return 0;
+}
+
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -125,8 +125,20 @@
 if (isUsedToInitializeAConstant(Result, Parent))
   return true;
 
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get() &&
+llvm::any_of(
+Result.Context->getParents(Parent),
+[](const DynTypedNode ) {
+  return GrandParent.get() !=
+ nullptr;
+}))
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
 if (Parent.get())
   return true;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D71686#1794330 , @aaron.ballman 
wrote:

> In D71686#1794053 , @0x8000- 
> wrote:
>
> > My take: this change fixes a user-reported bug, and does not cause any 
> > known regressions. I think we should integrate this.
>
>
> I sort of wonder whether we want to document this as a blessed approach to 
> silencing the warning. I'm not certain if it's too obtuse or not, but I 
> notice the check has no documented ways to silence the diagnostic aside from 
> using the correct kind of magic number or adding it to a list of excluded 
> magic numbers.


You mean Hyrum's Law  is not sufficient?

The check can be silenced with the regular NOLINT, or with defining and using a 
constant/enum. Using this "backdoor" way seems even more cumbersome and 
confusing than the NOLINT. At least with NOLINT it is clear what you're doing, 
and somebody else can grep for it and fix it if it is appropriate.


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

My take: this change fixes a user-reported bug, and does not cause any known 
regressions. I think we should integrate this.


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp:9
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]

This test fails even with clang-tidy-9.


```
$ clang-tidy-9 --checks="-*,readability-magic-numbers" 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp

/home/florin/tools/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp:9:35:
 warning: 10 is a magic number; consider replacing it with a named constant 
[readability-magic-numbers]
  if (((int)4) > ProcessSomething(10))
  ^
```

Nothing about the "4" here.


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 234998.
0x8000- added a comment.

Add a test file for expected failures


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

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]
 
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t
+// RUN: --
+// XFAIL: *
+
+int ProcessSomething(int input);
+
+int DoWork()
+{
+  if (((int)4) > ProcessSomething(10))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 4 is a magic number; consider 
replacing it with a named constant [readability-magic-numbers]
+return 0;
+
+   return 0;
+}
+
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -125,8 +125,20 @@
 if (isUsedToInitializeAConstant(Result, Parent))
   return true;
 
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get() &&
+llvm::any_of(
+Result.Context->getParents(Parent),
+[](const DynTypedNode ) {
+  return GrandParent.get() !=
+ nullptr;
+}))
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
 if (Parent.get())
   return true;
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -59,7 +59,7 @@
   const int anotherConstant;
 };
 
-int ValueArray[] = {3, 5};
+int ValueArray[] = {3, 5, 0, 0, 0};
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct 

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:129
+// expanded class enumeration value.
+if (Parent.get())
+  return true;

0x8000- wrote:
> aaron.ballman wrote:
> > So will this no longer treat *any* C-style cast as being a magic number? 
> > e.g., `int i; i = (int)12;`
> Sadly, it would seem so.
> 
> if (ValueArray[(int)4] > ValueArray[1]) produces the following AST:
> 
> ```
> |   | | | `-ArraySubscriptExpr 0x11545f0  'int' lvalue
> |   | | |   |-ImplicitCastExpr 0x11545d8  'int *' 
> |   | | |   | `-DeclRefExpr 0x1154558  'int [5]' lvalue Var 0x1153cb0 
> 'ValueArray' 'int [5]'
>
> |   | | |   `-CStyleCastExpr 0x11545b0  'int' 
> |   | | | `-IntegerLiteral 0x1154578  'int' 4
> ```
> 
> While the test case we're trying to avoid is:
> ```
> `-TemplateSpecializationType 0x1169ee0 'holder<(Letter)9U>' sugar holder
>   |-TemplateArgument expr
>   | `-ConstantExpr 0x1169dc0  'Letter' 9
>   |   `-SubstNonTypeTemplateParmExpr 0x1169da0  'Letter'
>   | `-CStyleCastExpr 0x1169d78  'Letter' 
>   |   `-IntegerLiteral 0x1169d48  'unsigned int' 9
>   `-RecordType 0x1169ec0 'holder'
> `-ClassTemplateSpecialization 0x1169de0 'holder'
> ```
> 
> Now, the question is - do we care? How many people use a C cast with a bare 
> integer?
> 
> I suppose I can add a check for the grandparent node to be 
> SubstNonTypeTemplateParmExpr, in order to filter this out?
An interesting fact is that the C-style cast does not produce a warning now 
anyway (even before applying this change.) and I don't have any idea why.

Adding these lines does not create a test failure:

```
+  79   if (((int)4) > IntSquarer[10])  
   
+  80 return 0;  
```


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:129
+// expanded class enumeration value.
+if (Parent.get())
+  return true;

aaron.ballman wrote:
> So will this no longer treat *any* C-style cast as being a magic number? 
> e.g., `int i; i = (int)12;`
Sadly, it would seem so.

if (ValueArray[(int)4] > ValueArray[1]) produces the following AST:

```
|   | | | `-ArraySubscriptExpr 0x11545f0  'int' lvalue
|   | | |   |-ImplicitCastExpr 0x11545d8  'int *' 
|   | | |   | `-DeclRefExpr 0x1154558  'int [5]' lvalue Var 0x1153cb0 
'ValueArray' 'int [5]'  
 
|   | | |   `-CStyleCastExpr 0x11545b0  'int' 
|   | | | `-IntegerLiteral 0x1154578  'int' 4
```

While the test case we're trying to avoid is:
```
`-TemplateSpecializationType 0x1169ee0 'holder<(Letter)9U>' sugar holder
  |-TemplateArgument expr
  | `-ConstantExpr 0x1169dc0  'Letter' 9
  |   `-SubstNonTypeTemplateParmExpr 0x1169da0  'Letter'
  | `-CStyleCastExpr 0x1169d78  'Letter' 
  |   `-IntegerLiteral 0x1169d48  'unsigned int' 9
  `-RecordType 0x1169ec0 'holder'
`-ClassTemplateSpecialization 0x1169de0 'holder'
```

Now, the question is - do we care? How many people use a C cast with a bare 
integer?

I suppose I can add a check for the grandparent node to be 
SubstNonTypeTemplateParmExpr, in order to filter this out?


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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 234819.
0x8000- added a comment.

Fix typo.


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

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the synthetically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult ,
const Expr ) const {
-  return llvm::any_of(
-  Result.Context->getParents(ExprResult),
-  [](const DynTypedNode ) {
-if (isUsedToInitializeAConstant(Result, Parent))
-  return true;
-
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
-if (Parent.get())
-  return true;
-
-// Don't warn on string user defined literals:
-// std::string s = "Hello World"s;
-if (const auto *UDL = Parent.get())
-  if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-return true;
-
-return false;
-  });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+  [](const DynTypedNode ) {
+if (isUsedToInitializeAConstant(Result, Parent))
+  return true;
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get())
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
+if (Parent.get())
+  return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+  if (UDL->getLiteralOperatorKind() ==
+  UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
+  });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 234818.
0x8000- edited the summary of this revision.
0x8000- added a comment.

Update release notes.


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

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@
   The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
   the warning for numbers used to specify bit field widths.
 
+  The check was updated to eliminate some false positives (such as using
+  class enumeration as non-type template parameters, or the syntethically
+  computed lengh of a static user string literal.)
+
 - New :doc:`readability-make-member-function-const
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult ,
const Expr ) const {
-  return llvm::any_of(
-  Result.Context->getParents(ExprResult),
-  [](const DynTypedNode ) {
-if (isUsedToInitializeAConstant(Result, Parent))
-  return true;
-
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
-if (Parent.get())
-  return true;
-
-// Don't warn on string user defined literals:
-// std::string s = "Hello World"s;
-if (const auto *UDL = Parent.get())
-  if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-return true;
-
-return false;
-  });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+  [](const DynTypedNode ) {
+if (isUsedToInitializeAConstant(Result, Parent))
+  return true;
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get())
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
+if (Parent.get())
+  return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+  if (UDL->getLiteralOperatorKind() ==
+  UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
+  });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/docs/ReleaseNotes.rst

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked an inline comment as done.
0x8000- added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:127
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.

127-130 are the new lines. The rest were moved about by clang-format. Please 
let me know if I should revert the format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71686



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

[PATCH] D71686: Fix false positive in magic number checker

[Florin Iucha via Phabricator via cfe-commits]

0x8000- created this revision.
0x8000- added a reviewer: aaron.ballman.
0x8000- added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix false positive in magic number checker

https://bugs.llvm.org/show_bug.cgi?id=40640: 
cppcoreguidelines-avoid-magic-numbers should not warn about enum class


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71686

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings (code by Pavel 
Kryukov)
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult ,
const Expr ) const {
-  return llvm::any_of(
-  Result.Context->getParents(ExprResult),
-  [](const DynTypedNode ) {
-if (isUsedToInitializeAConstant(Result, Parent))
-  return true;
-
-// Ignore this instance, because this match reports the location
-// where the template is defined, not where it is instantiated.
-if (Parent.get())
-  return true;
-
-// Don't warn on string user defined literals:
-// std::string s = "Hello World"s;
-if (const auto *UDL = Parent.get())
-  if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-return true;
-
-return false;
-  });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+  [](const DynTypedNode ) {
+if (isUsedToInitializeAConstant(Result, Parent))
+  return true;
+
+// Ignore this instance, because this matches an
+// expanded class enumeration value.
+if (Parent.get())
+  return true;
+
+// Ignore this instance, because this match reports the
+// location where the template is defined, not where it
+// is instantiated.
+if (Parent.get())
+  return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+  if (UDL->getLiteralOperatorKind() ==
+  UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
+  });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -215,3 +215,14 @@
 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings (code by Pavel Kryukov)
+enum class Letter : unsigned {
+A, B, C, D, E, F, G, H, I, J
+};
+
+template struct holder  { Letter letter = x;  };
+template struct wrapper { using h_type = holder;  };
+
+template struct wrapper;
+template struct wrapper;
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -34,7 +34,7 @@
 return AsDecl->isImplicit();
   }
 
-  if (Node.get() != nullptr)
+  if (Node.get())
 return true;
 
   return llvm::any_of(Result.Context->getParents(Node),
@@ -119,25 +119,31 @@
 
 bool MagicNumbersCheck::isConstant(const 

[PATCH] D67265: [clang-tidy] Magic number checker shouldn't warn on user defined string literals

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.
Herald added a subscriber: mgehre.

@aaron.ballman  - can you please review this patch and if you find it 
acceptable please integrate it?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D67265



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

[PATCH] D71043: Exclude bitfield definitions from magic numbers check

[Florin Iucha via Phabricator via cfe-commits]

0x8000- created this revision.
0x8000- added reviewers: JonasToth, lebedev.ri, aaron.ballman, alexfh.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
0x8000- removed a reviewer: lebedev.ri.
0x8000- edited projects, added clang-tools-extra; removed clang.
0x8000- edited subscribers, added: clang-tools-extra, Eugene.Zelenko, 
lebedev.ri; removed: cfe-commits.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71043

Files:
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-bitfields.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
@@ -79,6 +79,26 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 }
 
+struct HardwareGateway {
+   unsigned int Some: 5;
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   unsigned int Bits: 7;
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   unsigned int: 7;
+   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   unsigned int Rest: 13;
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 13 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+};
+
+unsigned int TestBitFields() {
+   HardwareGateway SomeRegister{};
+
+   SomeRegister.Some = 1;
+   SomeRegister.Rest = 2;
+
+   return SomeRegister.Bits;
+}
+
 /*
  * Clean code
  */
Index: clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-bitfields.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-bitfields.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoreBitFieldsWidths, value: 1}]}' \
+// RUN: --
+
+struct HardwareGateway {
+   unsigned int Some: 5;
+   unsigned int Bits: 7;
+   unsigned int: 7;
+   unsigned int Rest: 13;
+};
+
+unsigned int TestBitFields() {
+   HardwareGateway SomeRegister{};
+
+   SomeRegister.Some = 1;
+   SomeRegister.Rest = 22;
+   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+   return SomeRegister.Bits;
+}
+
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h
@@ -40,10 +40,17 @@
 const FloatingLiteral *) const {
 return false;
   }
-
   bool isSyntheticValue(const clang::SourceManager *SourceManager,
 const IntegerLiteral *Literal) const;
 
+  bool isBitFieldWidth(const clang::ast_matchers::MatchFinder::MatchResult &,
+   const FloatingLiteral &) const {
+ return false;
+  }
+
+  bool isBitFieldWidth(const clang::ast_matchers::MatchFinder::MatchResult ,
+   const IntegerLiteral ) const;
+
   template 
   void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult ,
const char *BoundName) {
@@ -64,6 +71,9 @@
 if (isSyntheticValue(Result.SourceManager, MatchedLiteral))
   return;
 
+if (isBitFieldWidth(Result, *MatchedLiteral))
+  return;
+
 const StringRef LiteralSourceText = Lexer::getSourceText(
 CharSourceRange::getTokenRange(MatchedLiteral->getSourceRange()),
 *Result.SourceManager, getLangOpts());
@@ -74,6 +84,7 @@
   }
 
   const bool IgnoreAllFloatingPointValues;
+  const bool IgnoreBitFieldsWidths;
   const bool IgnorePowersOf2IntegerValues;
 
   constexpr static unsigned SensibleNumberOfMagicValueExceptions = 16;
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -43,6 +43,19 @@
   });
 }
 
+bool 

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D45444#1766852 , @JonasToth wrote:

>




> Thank you for testing, I would appreciate if you test later versions, too!
>  I will focus on landing the utility function for adding `const` first. I 
> noticed false positives in LLVM as well, I try to reproduce them all and 
> fix/workaround the issues.

I will test it as often as you rebase it :) Thank you for the useful tool!

Are you going to keep updating it on 
https://github.com/JonasToth/llvm-project/commits/feature_check_const ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Thank you for rebasing on current master.

I have ran it today on our code base and found three classes of false positives:

1. Writing to a bitfield of a struct. The struct still is suggested it should 
be const.
2. Using a variable with an ostream extraction; like "int foo; cin >> foo;", 
except it was a template on the stream instance.
3. In a for loop, with a somewhat strange pair (for auto [foo, bar] = std::pair 
{}; foo < big_foo; ++ foo).

Let me know if you can't create test cases from these descriptions and I can 
try to extract the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45444



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

[PATCH] D67265: [clang-tidy] Magic number checker shouldn't warn on user defined string literals

[Florin Iucha via Phabricator via cfe-commits]

0x8000- accepted this revision.
0x8000- added a comment.
This revision is now accepted and ready to land.

Looks good to me. Thank you for the fix.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D67265



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In https://reviews.llvm.org/D49114#1196480, @aaron.ballman wrote:

> Alex has had plenty of time to respond, so I'm fine handling any concerns he 
> has post-commit. Do you need me to commit this on your behalf?


Yes, please! Thank you!

Author: "Florin Iucha "


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 160243.
0x8000- added a comment.

Rebased on curent master.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,199 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0;0x1.2p3"}, \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[15];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 22; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float SomeFloats[] = {0.5, 0x1.2p4};
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 0.5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: 0x1.2p4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 159222.
0x8000- added a comment.

Address several style comments. Rebase to current trunk (8.0).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,199 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0;0x1.2p3"}, \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[15];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 22; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float SomeFloats[] = {0.5, 0x1.2p4};
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 0.5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: 0x1.2p4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 158450.
0x8000- added a comment.

Add reference to 5.1.1 Use symbolic names instead of literal values in code 

 in the documentation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,199 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0;0x1.2p3"}, \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[15];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 22; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float SomeFloats[] = {0.5, 0x1.2p4};
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 0.5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: 0x1.2p4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 158425.
0x8000- added a comment.

Add tests to show that we can detect, report and ignore floating point numbers 
represented using hexadecimal notation


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,199 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0;0x1.2p3"}, \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[15];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 22; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float SomeFloats[] = {0.5, 0x1.2p4};
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 0.5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: 0x1.2p4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

The state of this patch:

- user interface is as agreed-upon, giving end-users the capability to filter 
out floats and ints as it makes sense for their code base
- code is clean
- documentation is up to date
- default ignore lists are sensible

There is one outstanding improvement request - to combine the two float/double 
lists into a single APFloat but that requires more refactoring outside this 
check and the benefit lies mainly in a future extensibility and the saving of a 
couple hundred bytes.

@aaron.ballman - can we get this merged as-is and improve it later? I'd like to 
see this check available in Clang7.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 157899.
0x8000- added a comment.

Update the list of magic values ignored by default.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}, \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[15];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 22; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100, 65536};
+
+float GrandfatheredFloatValues[] = {3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4};
+
+/*
+ * no warnings for enums
+ */
+enum Smorgasbord {
+  STARTER,
+  ALPHA = 3,
+  BETA = 1 << 5,
+};
+
+const 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

aaron.ballman wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > 0x8000- wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > 0x8000- wrote:
> > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > This is where I would construct an `APFloat` 
> > > > > > > > > > > > > > > > object from the string given. As for the 
> > > > > > > > > > > > > > > > semantics to be used, I would recommend getting 
> > > > > > > > > > > > > > > > it from `TargetInfo::getDoubleFormat()` on the 
> > > > > > > > > > > > > > > > belief that we aren't going to care about 
> > > > > > > > > > > > > > > > precision (explained in the documentation).
> > > > > > > > > > > > > > > Here is the problem I tried to explain last night 
> > > > > > > > > > > > > > > but perhaps I wasn't clear enough.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > When we parse the input list from strings, we 
> > > > > > > > > > > > > > > have to commit to one floating point value 
> > > > > > > > > > > > > > > "semantic" - in our case single or double 
> > > > > > > > > > > > > > > precision.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > When we encounter the value in the source code 
> > > > > > > > > > > > > > > and it is captured by a matcher, it comes as 
> > > > > > > > > > > > > > > either one of those values.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Floats with different semantics can't be directly 
> > > > > > > > > > > > > > > compared - so we have to maintain two distinct 
> > > > > > > > > > > > > > > arrays.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > If we do that, rather than store APFloats and 
> > > > > > > > > > > > > > > sort/compare them with awkward lambdas, we might 
> > > > > > > > > > > > > > > as well just use the native float/double and be 
> > > > > > > > > > > > > > > done with it more cleanly.
> > > > > > > > > > > > > > >When we encounter the value in the source code and 
> > > > > > > > > > > > > > >it is captured by a matcher, it comes as either 
> > > > > > > > > > > > > > >one of those values.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > It may also come in as long double or __float128, 
> > > > > > > > > > > > > > for instance, because there are type suffixes for 
> > > > > > > > > > > > > > that.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Floats with different semantics can't be directly 
> > > > > > > > > > > > > > > compared - so we have to maintain two distinct 
> > > > > > > > > > > > > > > arrays.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Yes, floats with different semantics cannot be 
> > > > > > > > > > > > > > directly compared. That's why I said below that we 
> > > > > > > > > > > > > > should coerce the literal values.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > If we do that, rather than store APFloats and 
> > > > > > > > > > > > > > > sort/compare them with awkward lambdas, we might 
> > > > > > > > > > > > > > > as well just use the native float/double and be 
> > > > > > > > > > > > > > > done with it more cleanly.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > There are too many different floating-point 
> > > > > > > > > > > > > > semantics for this to be viable, hence why coercion 
> > > > > > > > > > > > > > is a reasonable behavior.
> > > > > > > > > > > > > Let me see if I understood it - your proposal is: 
> > > > > > > > > > > > > store only doubles, and when a floating-point literal 
> > > > > > > > > > > > > is encountered in code, do not use the 
> > > > > > > > > > > > > FloatingLiteral instance, but parse it again into a 
> > > > > > > > > > > > > double and compare exactly. If the comparison matches 
> > > > > > > > > > > > > - ignore it.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > In that case what is the value of storing APFloats 
> > > > > > > > > > > > > with double semantics in the IgnoredValues array, 
> > > > > > > > > > > > > instead of doubles?
> > > > > > > > > > > > > Let me see if I understood it - your proposal is: 
> 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

> Based on this, I think the integer list should also include 2, 3, and 4 as 
> defaults -- those show up a lot more than I'd have expected. As for 
> floating-point values, 1.0 certainly jumps out at me, but none of the rest 
> seem particularly common. What do you think?

I'm good with 0, 1, 2, 3, 4 for integers and 0.0, 1.0 and also 100.0 (used to 
compute percentages) for floating point values.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

0x8000- wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > 0x8000- wrote:
> > > > > > 0x8000- wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > This is where I would construct an `APFloat` object 
> > > > > > > > > > > > > > from the string given. As for the semantics to be 
> > > > > > > > > > > > > > used, I would recommend getting it from 
> > > > > > > > > > > > > > `TargetInfo::getDoubleFormat()` on the belief that 
> > > > > > > > > > > > > > we aren't going to care about precision (explained 
> > > > > > > > > > > > > > in the documentation).
> > > > > > > > > > > > > Here is the problem I tried to explain last night but 
> > > > > > > > > > > > > perhaps I wasn't clear enough.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > When we parse the input list from strings, we have to 
> > > > > > > > > > > > > commit to one floating point value "semantic" - in 
> > > > > > > > > > > > > our case single or double precision.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > When we encounter the value in the source code and it 
> > > > > > > > > > > > > is captured by a matcher, it comes as either one of 
> > > > > > > > > > > > > those values.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Floats with different semantics can't be directly 
> > > > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > If we do that, rather than store APFloats and 
> > > > > > > > > > > > > sort/compare them with awkward lambdas, we might as 
> > > > > > > > > > > > > well just use the native float/double and be done 
> > > > > > > > > > > > > with it more cleanly.
> > > > > > > > > > > > >When we encounter the value in the source code and it 
> > > > > > > > > > > > >is captured by a matcher, it comes as either one of 
> > > > > > > > > > > > >those values.
> > > > > > > > > > > > 
> > > > > > > > > > > > It may also come in as long double or __float128, for 
> > > > > > > > > > > > instance, because there are type suffixes for that.
> > > > > > > > > > > > 
> > > > > > > > > > > > > Floats with different semantics can't be directly 
> > > > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > > > 
> > > > > > > > > > > > Yes, floats with different semantics cannot be directly 
> > > > > > > > > > > > compared. That's why I said below that we should coerce 
> > > > > > > > > > > > the literal values.
> > > > > > > > > > > > 
> > > > > > > > > > > > > If we do that, rather than store APFloats and 
> > > > > > > > > > > > > sort/compare them with awkward lambdas, we might as 
> > > > > > > > > > > > > well just use the native float/double and be done 
> > > > > > > > > > > > > with it more cleanly.
> > > > > > > > > > > > 
> > > > > > > > > > > > There are too many different floating-point semantics 
> > > > > > > > > > > > for this to be viable, hence why coercion is a 
> > > > > > > > > > > > reasonable behavior.
> > > > > > > > > > > Let me see if I understood it - your proposal is: store 
> > > > > > > > > > > only doubles, and when a floating-point literal is 
> > > > > > > > > > > encountered in code, do not use the FloatingLiteral 
> > > > > > > > > > > instance, but parse it again into a double and compare 
> > > > > > > > > > > exactly. If the comparison matches - ignore it.
> > > > > > > > > > > 
> > > > > > > > > > > In that case what is the value of storing APFloats with 
> > > > > > > > > > > double semantics in the IgnoredValues array, instead of 
> > > > > > > > > > > doubles?
> > > > > > > > > > > Let me see if I understood it - your proposal is: store 
> > > > > > > > > > > only doubles, and when a floating-point literal is 
> > > > > > > > > > > encountered in code, do not use the FloatingLiteral 
> > > > > > > > > > > instance, but parse it again into a double and compare 
> > > > > > > > > > > exactly. If the comparison matches - ignore it.
> > > > > > > > > > 
> > > > > > > > > > My proposal is to use `APFloat` as the storage and 
> > > > > > > > > > comparison medium. Read in strings from the configuration 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

aaron.ballman wrote:
> aaron.ballman wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > 0x8000- wrote:
> > > > > 0x8000- wrote:
> > > > > > 0x8000- wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > This is where I would construct an `APFloat` object 
> > > > > > > > > > > > from the string given. As for the semantics to be used, 
> > > > > > > > > > > > I would recommend getting it from 
> > > > > > > > > > > > `TargetInfo::getDoubleFormat()` on the belief that we 
> > > > > > > > > > > > aren't going to care about precision (explained in the 
> > > > > > > > > > > > documentation).
> > > > > > > > > > > Here is the problem I tried to explain last night but 
> > > > > > > > > > > perhaps I wasn't clear enough.
> > > > > > > > > > > 
> > > > > > > > > > > When we parse the input list from strings, we have to 
> > > > > > > > > > > commit to one floating point value "semantic" - in our 
> > > > > > > > > > > case single or double precision.
> > > > > > > > > > > 
> > > > > > > > > > > When we encounter the value in the source code and it is 
> > > > > > > > > > > captured by a matcher, it comes as either one of those 
> > > > > > > > > > > values.
> > > > > > > > > > > 
> > > > > > > > > > > Floats with different semantics can't be directly 
> > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > > 
> > > > > > > > > > > If we do that, rather than store APFloats and 
> > > > > > > > > > > sort/compare them with awkward lambdas, we might as well 
> > > > > > > > > > > just use the native float/double and be done with it more 
> > > > > > > > > > > cleanly.
> > > > > > > > > > >When we encounter the value in the source code and it is 
> > > > > > > > > > >captured by a matcher, it comes as either one of those 
> > > > > > > > > > >values.
> > > > > > > > > > 
> > > > > > > > > > It may also come in as long double or __float128, for 
> > > > > > > > > > instance, because there are type suffixes for that.
> > > > > > > > > > 
> > > > > > > > > > > Floats with different semantics can't be directly 
> > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > 
> > > > > > > > > > Yes, floats with different semantics cannot be directly 
> > > > > > > > > > compared. That's why I said below that we should coerce the 
> > > > > > > > > > literal values.
> > > > > > > > > > 
> > > > > > > > > > > If we do that, rather than store APFloats and 
> > > > > > > > > > > sort/compare them with awkward lambdas, we might as well 
> > > > > > > > > > > just use the native float/double and be done with it more 
> > > > > > > > > > > cleanly.
> > > > > > > > > > 
> > > > > > > > > > There are too many different floating-point semantics for 
> > > > > > > > > > this to be viable, hence why coercion is a reasonable 
> > > > > > > > > > behavior.
> > > > > > > > > Let me see if I understood it - your proposal is: store only 
> > > > > > > > > doubles, and when a floating-point literal is encountered in 
> > > > > > > > > code, do not use the FloatingLiteral instance, but parse it 
> > > > > > > > > again into a double and compare exactly. If the comparison 
> > > > > > > > > matches - ignore it.
> > > > > > > > > 
> > > > > > > > > In that case what is the value of storing APFloats with 
> > > > > > > > > double semantics in the IgnoredValues array, instead of 
> > > > > > > > > doubles?
> > > > > > > > > Let me see if I understood it - your proposal is: store only 
> > > > > > > > > doubles, and when a floating-point literal is encountered in 
> > > > > > > > > code, do not use the FloatingLiteral instance, but parse it 
> > > > > > > > > again into a double and compare exactly. If the comparison 
> > > > > > > > > matches - ignore it.
> > > > > > > > 
> > > > > > > > My proposal is to use `APFloat` as the storage and comparison 
> > > > > > > > medium. Read in strings from the configuration and convert them 
> > > > > > > > to an `APFloat` that has double semantics. Read in literals and 
> > > > > > > > call `FloatLiteral::getValue()` to get the `APFloat` from it, 
> > > > > > > > convert it to one that has double semantics as needed, then 
> > > > > > > > perform the comparison between those two `APFloat` objects.
> > > > 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Top 40 magic numbers in https://github.com/qt/qtbase

  4859 2
  2901 3
  1855 4
   985 5
   968 8
   605 6
   600 7
   439 16
   432 10
   363 
   356 32
   241 1.0f
   217 12
   209 255
   207 100
   205 9
   205 20
   204 50
   177 0.5
   174 15
   162 0x2
   144 24
   140 0x80
   135 11
   127 256
   113 14
   110 0xff
   101 1.0
99 64
99 200
96 13
86 30
84 1000
68 18
66 150
62 127
62 0xFF
58 19
58 0.05f
57 128

Top 40 floating point magic numbers in https://github.com/qt/qtbase

  241 1.0f
  177 0.5
  101 1.0
   58 0.05f
   44 2.0
   42 0.5f
   31 10.0
   28 30.0
   24 20.0
   22 60.0
   20 100.0
   19 0.8
   19 0.25
   17 0.2
   16 1000.0
   14 1.224744871
   14 100.
   13 25.0
   13 0.1
   12 90.0
   12 40.0
   12 0.707106781
   12 0.30
   12 0.20
   11 80.0
   11 6.0
   11 50.0
   11 2.0f
   11 0.75
   11 0.66f
   11 0.1f
   10 6.28
   10 5.0
   10 4.0
   10 1.414213562
9 360.0
9 25.4
9 2.54
8 70.0
8 55.0

Top 40 magic numbers in https://github.com/facebook/rocksdb

  2131 2
   896 3
   859 4
   858 10
   685 100
   678 1024
   600 8
   445 5
   323 1000
   244 20
   231 301
   227 200
   223 6
   209 16
   189 7
   154 1
   131 100
   119 10
   111 30
   105 256
   104 32
   103 5U
   103 50
94 128
91 64
89 60
88 3U
85 2U
84 500
72 4U
67 9
65 300
63 13
59 0xff
57 6U
52 4096
52 24
52 12
51 600
50 10U

Top 40 floating point numbers in rocksdb:

  37 100.0
  30 1.0
  27 0.5
  24 0.001
  12 1048576.0
  12 0.25
  11 1.1
   8 50.0
   8 1.5
   8 1.0
   5 .3
   5 .1
   5 0.8
   4 99.99
   4 99.9
   4 2.0
   4 1.048576
   4 100.0f
   4 0.9
   4 0.75
   4 0.69
   4 0.02
   4 0.1
   3 100.0
   3 0.4
   3 0.1
   2 0.7
   2 0.6
   2 0.45
   1 8.0
   1 5.6
   1 40.2
   1 40.1
   1 3.25
   1 2.0
   1 2.
   1 116.2
   1 116.1
   1 110.5e-4
   1 1024.0


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 157889.
0x8000- added a comment.

Add a flag to ignore all powers of two integral values.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}, \
+// RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[15];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 22; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100, 65536};
+
+float GrandfatheredFloatValues[] = {3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4};
+
+/*
+ * no warnings for enums
+ */
+enum Smorgasbord {
+  STARTER,
+  ALPHA = 3,
+  BETA = 1 << 5,
+};
+

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

0x8000- wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > 0x8000- wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > This is where I would construct an `APFloat` object from the 
> > > > > > > > > string given. As for the semantics to be used, I would 
> > > > > > > > > recommend getting it from `TargetInfo::getDoubleFormat()` on 
> > > > > > > > > the belief that we aren't going to care about precision 
> > > > > > > > > (explained in the documentation).
> > > > > > > > Here is the problem I tried to explain last night but perhaps I 
> > > > > > > > wasn't clear enough.
> > > > > > > > 
> > > > > > > > When we parse the input list from strings, we have to commit to 
> > > > > > > > one floating point value "semantic" - in our case single or 
> > > > > > > > double precision.
> > > > > > > > 
> > > > > > > > When we encounter the value in the source code and it is 
> > > > > > > > captured by a matcher, it comes as either one of those values.
> > > > > > > > 
> > > > > > > > Floats with different semantics can't be directly compared - so 
> > > > > > > > we have to maintain two distinct arrays.
> > > > > > > > 
> > > > > > > > If we do that, rather than store APFloats and sort/compare them 
> > > > > > > > with awkward lambdas, we might as well just use the native 
> > > > > > > > float/double and be done with it more cleanly.
> > > > > > > >When we encounter the value in the source code and it is 
> > > > > > > >captured by a matcher, it comes as either one of those values.
> > > > > > > 
> > > > > > > It may also come in as long double or __float128, for instance, 
> > > > > > > because there are type suffixes for that.
> > > > > > > 
> > > > > > > > Floats with different semantics can't be directly compared - so 
> > > > > > > > we have to maintain two distinct arrays.
> > > > > > > 
> > > > > > > Yes, floats with different semantics cannot be directly compared. 
> > > > > > > That's why I said below that we should coerce the literal values.
> > > > > > > 
> > > > > > > > If we do that, rather than store APFloats and sort/compare them 
> > > > > > > > with awkward lambdas, we might as well just use the native 
> > > > > > > > float/double and be done with it more cleanly.
> > > > > > > 
> > > > > > > There are too many different floating-point semantics for this to 
> > > > > > > be viable, hence why coercion is a reasonable behavior.
> > > > > > Let me see if I understood it - your proposal is: store only 
> > > > > > doubles, and when a floating-point literal is encountered in code, 
> > > > > > do not use the FloatingLiteral instance, but parse it again into a 
> > > > > > double and compare exactly. If the comparison matches - ignore it.
> > > > > > 
> > > > > > In that case what is the value of storing APFloats with double 
> > > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > > Let me see if I understood it - your proposal is: store only 
> > > > > > doubles, and when a floating-point literal is encountered in code, 
> > > > > > do not use the FloatingLiteral instance, but parse it again into a 
> > > > > > double and compare exactly. If the comparison matches - ignore it.
> > > > > 
> > > > > My proposal is to use `APFloat` as the storage and comparison medium. 
> > > > > Read in strings from the configuration and convert them to an 
> > > > > `APFloat` that has double semantics. Read in literals and call 
> > > > > `FloatLiteral::getValue()` to get the `APFloat` from it, convert it 
> > > > > to one that has double semantics as needed, then perform the 
> > > > > comparison between those two `APFloat` objects.
> > > > > 
> > > > > > In that case what is the value of storing APFloats with double 
> > > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > 
> > > > > Mostly that it allows us to modify or extend the check for more 
> > > > > complicated semantics in the future. Also, it's good practice to use 
> > > > > something with consistent semantic behavior across hosts and targets 
> > > > > (comparisons between numbers that cannot be precisely represented 
> > > > > will at least be consistently compared across hosts when compiling 
> > > > > for the same target).
> > > > > 
> > > > ok - coming right up!
> > > > My proposal is to use APFloat as the storage 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

0x8000- wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > aaron.ballman wrote:
> > > > > > 0x8000- wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > This is where I would construct an `APFloat` object from the 
> > > > > > > > string given. As for the semantics to be used, I would 
> > > > > > > > recommend getting it from `TargetInfo::getDoubleFormat()` on 
> > > > > > > > the belief that we aren't going to care about precision 
> > > > > > > > (explained in the documentation).
> > > > > > > Here is the problem I tried to explain last night but perhaps I 
> > > > > > > wasn't clear enough.
> > > > > > > 
> > > > > > > When we parse the input list from strings, we have to commit to 
> > > > > > > one floating point value "semantic" - in our case single or 
> > > > > > > double precision.
> > > > > > > 
> > > > > > > When we encounter the value in the source code and it is captured 
> > > > > > > by a matcher, it comes as either one of those values.
> > > > > > > 
> > > > > > > Floats with different semantics can't be directly compared - so 
> > > > > > > we have to maintain two distinct arrays.
> > > > > > > 
> > > > > > > If we do that, rather than store APFloats and sort/compare them 
> > > > > > > with awkward lambdas, we might as well just use the native 
> > > > > > > float/double and be done with it more cleanly.
> > > > > > >When we encounter the value in the source code and it is captured 
> > > > > > >by a matcher, it comes as either one of those values.
> > > > > > 
> > > > > > It may also come in as long double or __float128, for instance, 
> > > > > > because there are type suffixes for that.
> > > > > > 
> > > > > > > Floats with different semantics can't be directly compared - so 
> > > > > > > we have to maintain two distinct arrays.
> > > > > > 
> > > > > > Yes, floats with different semantics cannot be directly compared. 
> > > > > > That's why I said below that we should coerce the literal values.
> > > > > > 
> > > > > > > If we do that, rather than store APFloats and sort/compare them 
> > > > > > > with awkward lambdas, we might as well just use the native 
> > > > > > > float/double and be done with it more cleanly.
> > > > > > 
> > > > > > There are too many different floating-point semantics for this to 
> > > > > > be viable, hence why coercion is a reasonable behavior.
> > > > > Let me see if I understood it - your proposal is: store only doubles, 
> > > > > and when a floating-point literal is encountered in code, do not use 
> > > > > the FloatingLiteral instance, but parse it again into a double and 
> > > > > compare exactly. If the comparison matches - ignore it.
> > > > > 
> > > > > In that case what is the value of storing APFloats with double 
> > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > Let me see if I understood it - your proposal is: store only doubles, 
> > > > > and when a floating-point literal is encountered in code, do not use 
> > > > > the FloatingLiteral instance, but parse it again into a double and 
> > > > > compare exactly. If the comparison matches - ignore it.
> > > > 
> > > > My proposal is to use `APFloat` as the storage and comparison medium. 
> > > > Read in strings from the configuration and convert them to an `APFloat` 
> > > > that has double semantics. Read in literals and call 
> > > > `FloatLiteral::getValue()` to get the `APFloat` from it, convert it to 
> > > > one that has double semantics as needed, then perform the comparison 
> > > > between those two `APFloat` objects.
> > > > 
> > > > > In that case what is the value of storing APFloats with double 
> > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > 
> > > > Mostly that it allows us to modify or extend the check for more 
> > > > complicated semantics in the future. Also, it's good practice to use 
> > > > something with consistent semantic behavior across hosts and targets 
> > > > (comparisons between numbers that cannot be precisely represented will 
> > > > at least be consistently compared across hosts when compiling for the 
> > > > same target).
> > > > 
> > > ok - coming right up!
> > > My proposal is to use APFloat as the storage and comparison medium. Read 
> > > in strings from the configuration and convert them to an APFloat that has 
> > > double semantics.
> > 
> > This is easy.
> > 
> > > Read in 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

0x8000- wrote:
> aaron.ballman wrote:
> > 0x8000- wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > aaron.ballman wrote:
> > > > > > This is where I would construct an `APFloat` object from the string 
> > > > > > given. As for the semantics to be used, I would recommend getting 
> > > > > > it from `TargetInfo::getDoubleFormat()` on the belief that we 
> > > > > > aren't going to care about precision (explained in the 
> > > > > > documentation).
> > > > > Here is the problem I tried to explain last night but perhaps I 
> > > > > wasn't clear enough.
> > > > > 
> > > > > When we parse the input list from strings, we have to commit to one 
> > > > > floating point value "semantic" - in our case single or double 
> > > > > precision.
> > > > > 
> > > > > When we encounter the value in the source code and it is captured by 
> > > > > a matcher, it comes as either one of those values.
> > > > > 
> > > > > Floats with different semantics can't be directly compared - so we 
> > > > > have to maintain two distinct arrays.
> > > > > 
> > > > > If we do that, rather than store APFloats and sort/compare them with 
> > > > > awkward lambdas, we might as well just use the native float/double 
> > > > > and be done with it more cleanly.
> > > > >When we encounter the value in the source code and it is captured by a 
> > > > >matcher, it comes as either one of those values.
> > > > 
> > > > It may also come in as long double or __float128, for instance, because 
> > > > there are type suffixes for that.
> > > > 
> > > > > Floats with different semantics can't be directly compared - so we 
> > > > > have to maintain two distinct arrays.
> > > > 
> > > > Yes, floats with different semantics cannot be directly compared. 
> > > > That's why I said below that we should coerce the literal values.
> > > > 
> > > > > If we do that, rather than store APFloats and sort/compare them with 
> > > > > awkward lambdas, we might as well just use the native float/double 
> > > > > and be done with it more cleanly.
> > > > 
> > > > There are too many different floating-point semantics for this to be 
> > > > viable, hence why coercion is a reasonable behavior.
> > > Let me see if I understood it - your proposal is: store only doubles, and 
> > > when a floating-point literal is encountered in code, do not use the 
> > > FloatingLiteral instance, but parse it again into a double and compare 
> > > exactly. If the comparison matches - ignore it.
> > > 
> > > In that case what is the value of storing APFloats with double semantics 
> > > in the IgnoredValues array, instead of doubles?
> > > Let me see if I understood it - your proposal is: store only doubles, and 
> > > when a floating-point literal is encountered in code, do not use the 
> > > FloatingLiteral instance, but parse it again into a double and compare 
> > > exactly. If the comparison matches - ignore it.
> > 
> > My proposal is to use `APFloat` as the storage and comparison medium. Read 
> > in strings from the configuration and convert them to an `APFloat` that has 
> > double semantics. Read in literals and call `FloatLiteral::getValue()` to 
> > get the `APFloat` from it, convert it to one that has double semantics as 
> > needed, then perform the comparison between those two `APFloat` objects.
> > 
> > > In that case what is the value of storing APFloats with double semantics 
> > > in the IgnoredValues array, instead of doubles?
> > 
> > Mostly that it allows us to modify or extend the check for more complicated 
> > semantics in the future. Also, it's good practice to use something with 
> > consistent semantic behavior across hosts and targets (comparisons between 
> > numbers that cannot be precisely represented will at least be consistently 
> > compared across hosts when compiling for the same target).
> > 
> ok - coming right up!
> My proposal is to use APFloat as the storage and comparison medium. Read in 
> strings from the configuration and convert them to an APFloat that has double 
> semantics.

This is easy.

> Read in literals and call FloatLiteral::getValue() to get the APFloat from 
> it, 

this is easy as well,

> convert it to one that has double semantics as needed, then perform the 
> comparison between those two APFloat objects.

The conversion methods in `APFloat` only produce plain-old-data-types: 
```
  double convertToDouble() const;   

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > I would still like to see some data on common floating-point literal 
> > > > > values used in large open source project so that we can see what 
> > > > > sensible values should be in this list.
> > > > What value would that bring? The ideal target is that there are no 
> > > > magic values - no guideline that I have seen makes exception for 3.141 
> > > > or 9.81. Each project is special based on how they evolved, and they 
> > > > need to decide for themselves what is worth cleaning vs what can be 
> > > > swept under the rug for now. Why would we lend authority to any 
> > > > particular floating point value?
> > > Because that's too high of a high false positive rate for an acceptable 
> > > clang-tidy check. As mentioned before, there are literally hundreds of 
> > > unnameable floating-point literals in LLVM alone where the value is 1.0 
> > > or 2.0. Having statistical data to pick sensible defaults for this list 
> > > is valuable in that it lowers the false positive rate. If the user 
> > > dislikes the default list for some reason (because for their project, 
> > > maybe 2.0 is a supremely nameable literal value), they can pick a 
> > > different set of defaults.
> > > 
> > > Right now, I'm operating off an assumption that most floating-point 
> > > literals that should not be named are going to be whole numbers that are 
> > > precisely represented in all floating-point semantic models. This data 
> > > will tell us if that assumption is wrong, and if the assumption is wrong, 
> > > we might want to go with separate lists like you've done.
> > Here are the results with the check as-is, run on the llvm code base as of 
> > last night:
> > 
> > top-40
> > ```
> >   10435 2
> >5543 4
> >4629 8
> >3271 3
> >2702 16
> >1876 32
> >1324 64
> >1309 10
> >1207 5
> >1116 128
> > 966 6
> > 733 7
> > 575 256
> > 421 20
> > 406 12
> > 339 9
> > 331 1024
> > 311 100
> > 281 42
> > 253 11
> > 226 15
> > 189 40
> > 172 24
> > 169 0xff
> > 168 13
> > 168 0x80
> > 166 512
> > 137 1.0
> > 133 14
> > 132 31
> > 129 0xDEADBEEF
> > 120 18
> > 120 17
> > 120 1000
> > 115 4096
> > 100 30
> >  94 60
> >  94 0x1234
> >  89 0x20
> >  86 0xFF
> > ```
> > 
> > 1.0 is in position 28 with 137 occurrences
> > 2.0 is in position 93 with 27 occurrences
> > 100.0 is in position 96 with 26 occurences
> > 1.0f is in position 182 with 11 occurences
> > 
> > we also have 2.0e0 four times :)
> > 
> > This data suggests that there would be value in a 
> > IgnorePowerOf2IntegerLiterals option.
> Any chance you can hack the check run on LLVM where it doesn't report any 
> integer values (I'm curious about the top ten or so for floating-point 
> values)? Additionally, it'd be great to get similar numbers from some other 
> projects, like https://github.com/qt just to see how it compares (I've used 
> `bear` to get compile_commands.json file out of Qt before, so I would guess 
> that would work here).
No need for the hack, I just grep for dot in my report:

```
137 1.0
 27 2.0
 26 100.0
 20 0.5
 11 1.0f
  8 1.02
  7 3.0
  7 1.98
  7 1.5
  6 .5
  6 1.1
  5 0.01
  4 89.0f
  4 4294967296.f
  4 3.14159
  4 2.0e0
  4 10.0
  3 88.0f
  3 255.0
  3 127.0f
  3 12345.0f
  3 123.45
  3 0.2f
  3 0.25
  3 0.1f
  3 0.1
  2 80.0
  2 710.0f
  2 710.0
  2 709.0f
  2 6.0
  2 3.14f
  2 3.14
  2 2.5
  2 2349214918.58107
  2 149.0f
  2 14.5f
  2 1.17549435e-38F
  2 11357.0f
  2 11356.0f
  2 103.0f
  2 0.99
  2 0.9
  2 0.01f
  1 745.0f
  1 745.0
  1 7.1653228759765625e2f
  1 709.0
  1 7.08687663657301358331704585496e-268
  1 6.62E-34
  1 64.0f
  1 6.02E23
  1 4950.0f
  1 4932.0f
  1 45.0f
  1 42.42
  1 4.2
  1 4.0
  1 38.0f
  1 3.7
  1 323.0f
  1 32.0f
  1 32.0
  1 3.1415
  1 308.0f
  1 2.718
  1 2.7
  1 225.0f
  1 21.67
  1 2.0f
  1 2.088
  1 .17532f
  1 16445.0f
  1 1.616f
  1 128.0f
  1 12346.0f
  1 12.0f
  1 1.2
  1 1.1f
  1 11399.0f
  1 11383.0f
  1 1.0L
  1 1.0E+9
  1 1.0E+6
  1 1.0e-5f
  1 1.0E+12
  1 1074.0f
  1 1074.0
  1 1023.0f
  1 1023.0
  1 1.01F
  1 1.01
  1 10.0f
  1 100.
  1 0.99
  1 0.8f
  1 .08215f
  1 0.7
  1 0.6
  1 0.5F
   

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > I would still like to see some data on common floating-point literal 
> > > values used in large open source project so that we can see what sensible 
> > > values should be in this list.
> > What value would that bring? The ideal target is that there are no magic 
> > values - no guideline that I have seen makes exception for 3.141 or 9.81. 
> > Each project is special based on how they evolved, and they need to decide 
> > for themselves what is worth cleaning vs what can be swept under the rug 
> > for now. Why would we lend authority to any particular floating point value?
> Because that's too high of a high false positive rate for an acceptable 
> clang-tidy check. As mentioned before, there are literally hundreds of 
> unnameable floating-point literals in LLVM alone where the value is 1.0 or 
> 2.0. Having statistical data to pick sensible defaults for this list is 
> valuable in that it lowers the false positive rate. If the user dislikes the 
> default list for some reason (because for their project, maybe 2.0 is a 
> supremely nameable literal value), they can pick a different set of defaults.
> 
> Right now, I'm operating off an assumption that most floating-point literals 
> that should not be named are going to be whole numbers that are precisely 
> represented in all floating-point semantic models. This data will tell us if 
> that assumption is wrong, and if the assumption is wrong, we might want to go 
> with separate lists like you've done.
Here are the results with the check as-is, run on the llvm code base as of last 
night:

top-40
```
  10435 2
   5543 4
   4629 8
   3271 3
   2702 16
   1876 32
   1324 64
   1309 10
   1207 5
   1116 128
966 6
733 7
575 256
421 20
406 12
339 9
331 1024
311 100
281 42
253 11
226 15
189 40
172 24
169 0xff
168 13
168 0x80
166 512
137 1.0
133 14
132 31
129 0xDEADBEEF
120 18
120 17
120 1000
115 4096
100 30
 94 60
 94 0x1234
 89 0x20
 86 0xFF
```

1.0 is in position 28 with 137 occurrences
2.0 is in position 93 with 27 occurrences
100.0 is in position 96 with 26 occurences
1.0f is in position 182 with 11 occurences

we also have 2.0e0 four times :)

This data suggests that there would be value in a IgnorePowerOf2IntegerLiterals 
option.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > This is where I would construct an `APFloat` object from the string 
> > > > > given. As for the semantics to be used, I would recommend getting it 
> > > > > from `TargetInfo::getDoubleFormat()` on the belief that we aren't 
> > > > > going to care about precision (explained in the documentation).
> > > > Here is the problem I tried to explain last night but perhaps I wasn't 
> > > > clear enough.
> > > > 
> > > > When we parse the input list from strings, we have to commit to one 
> > > > floating point value "semantic" - in our case single or double 
> > > > precision.
> > > > 
> > > > When we encounter the value in the source code and it is captured by a 
> > > > matcher, it comes as either one of those values.
> > > > 
> > > > Floats with different semantics can't be directly compared - so we have 
> > > > to maintain two distinct arrays.
> > > > 
> > > > If we do that, rather than store APFloats and sort/compare them with 
> > > > awkward lambdas, we might as well just use the native float/double and 
> > > > be done with it more cleanly.
> > > >When we encounter the value in the source code and it is captured by a 
> > > >matcher, it comes as either one of those values.
> > > 
> > > It may also come in as long double or __float128, for instance, because 
> > > there are type suffixes for that.
> > > 
> > > > Floats with different semantics can't be directly compared - so we have 
> > > > to maintain two distinct arrays.
> > > 
> > > Yes, floats with different semantics cannot be directly compared. That's 
> > > why I said below that we should coerce the literal values.
> > > 
> > > > If we do that, rather than store APFloats and sort/compare them with 
> > 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked 2 inline comments as done.
0x8000- added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > This is where I would construct an `APFloat` object from the string 
> > > given. As for the semantics to be used, I would recommend getting it from 
> > > `TargetInfo::getDoubleFormat()` on the belief that we aren't going to 
> > > care about precision (explained in the documentation).
> > Here is the problem I tried to explain last night but perhaps I wasn't 
> > clear enough.
> > 
> > When we parse the input list from strings, we have to commit to one 
> > floating point value "semantic" - in our case single or double precision.
> > 
> > When we encounter the value in the source code and it is captured by a 
> > matcher, it comes as either one of those values.
> > 
> > Floats with different semantics can't be directly compared - so we have to 
> > maintain two distinct arrays.
> > 
> > If we do that, rather than store APFloats and sort/compare them with 
> > awkward lambdas, we might as well just use the native float/double and be 
> > done with it more cleanly.
> >When we encounter the value in the source code and it is captured by a 
> >matcher, it comes as either one of those values.
> 
> It may also come in as long double or __float128, for instance, because there 
> are type suffixes for that.
> 
> > Floats with different semantics can't be directly compared - so we have to 
> > maintain two distinct arrays.
> 
> Yes, floats with different semantics cannot be directly compared. That's why 
> I said below that we should coerce the literal values.
> 
> > If we do that, rather than store APFloats and sort/compare them with 
> > awkward lambdas, we might as well just use the native float/double and be 
> > done with it more cleanly.
> 
> There are too many different floating-point semantics for this to be viable, 
> hence why coercion is a reasonable behavior.
Let me see if I understood it - your proposal is: store only doubles, and when 
a floating-point literal is encountered in code, do not use the FloatingLiteral 
instance, but parse it again into a double and compare exactly. If the 
comparison matches - ignore it.

In that case what is the value of storing APFloats with double semantics in the 
IgnoredValues array, instead of doubles?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- marked 4 inline comments as done.
0x8000- added a comment.

See inline comments. Basically we need two arrays because APFloats of different 
semantics don't compare well, and even if we coerce them, they sometimes are 
not equal.




Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+

aaron.ballman wrote:
> I would still like to see some data on common floating-point literal values 
> used in large open source project so that we can see what sensible values 
> should be in this list.
What value would that bring? The ideal target is that there are no magic values 
- no guideline that I have seen makes exception for 3.141 or 9.81. Each project 
is special based on how they evolved, and they need to decide for themselves 
what is worth cleaning vs what can be swept under the rug for now. Why would we 
lend authority to any particular floating point value?



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+  for (const auto  : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+

aaron.ballman wrote:
> This is where I would construct an `APFloat` object from the string given. As 
> for the semantics to be used, I would recommend getting it from 
> `TargetInfo::getDoubleFormat()` on the belief that we aren't going to care 
> about precision (explained in the documentation).
Here is the problem I tried to explain last night but perhaps I wasn't clear 
enough.

When we parse the input list from strings, we have to commit to one floating 
point value "semantic" - in our case single or double precision.

When we encounter the value in the source code and it is captured by a matcher, 
it comes as either one of those values.

Floats with different semantics can't be directly compared - so we have to 
maintain two distinct arrays.

If we do that, rather than store APFloats and sort/compare them with awkward 
lambdas, we might as well just use the native float/double and be done with it 
more cleanly.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:128-139
+  if (FloatValue.isZero())
+return true;
+  else if (() == ::APFloat::IEEEsingle()) {
+const float Value = FloatValue.convertToFloat();
+return std::binary_search(IgnoredFloatingPointValues.begin(),
+  IgnoredFloatingPointValues.end(), Value);
+  } else if (() == ::APFloat::IEEEdouble()) {

aaron.ballman wrote:
> Here is where I would compare `FloatValue` against everything in the 
> `IgnoredFloatingPointValues` array using `APFloat::compare()`. However, you 
> need the floats to have the same semantics for that to work, so you'd have to 
> convert `FloatValue` using `APFloat::convert()` to whatever the target 
> floating-point format is.
If you do that, 3.14f will not be equal to 3.14 . You need two arrays.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 157886.
0x8000- added a comment.

Indicate that `0` and `0.0` are accepted unconditionally (because it makes 
sense in the source code, and speeds-up many checks as 0s are very common and 
we don't want to spend log2(n) to find them at the beginning of the vector).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,194 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[8];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 8; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100};
+
+float GrandfatheredFloatValues[] = { 3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4 };
+
+/*
+ * no warnings for enums
+ */
+enum 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- updated this revision to Diff 157879.
0x8000- added a comment.

Add support for ignoring specific floating point numbers.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114

Files:
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MagicNumbersCheck.cpp
  clang-tidy/readability/MagicNumbersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-magic-numbers.rst
  test/clang-tidy/readability-magic-numbers.cpp

Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,194 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}]}' \
+// RUN: --
+
+template 
+struct ValueBucket {
+  T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+  return param * param;
+}
+
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  (void)IntSquarer(7);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int LocalArray[8];
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  for (int ii = 0; ii < 8; ++ii)
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+  }
+
+  ValueBucket Bucket;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+  TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int getValue() const;
+
+private:
+  int oneMember = 9;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+  int anotherMember;
+
+  int yetAnotherMember;
+
+  const int oneConstant = 2;
+
+  const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+  if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+  return -3; // FILENOTFOUND
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+  const int GoodLocalIntConstant = 43;
+
+  (void)IntSquarer(GoodLocalIntConstant);
+
+  int LocalArray[INT_MACRO];
+
+  ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100};
+
+float GrandfatheredFloatValues[] = { 3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4 };
+
+/*
+ * no warnings for enums
+ */
+enum Smorgasbord {
+  STARTER,
+  ALPHA = 3,
+  BETA = 1 << 5,
+};
+
+const float FloatPiConstant = 3.1415926535f;
+const double DoublePiConstant = 6.283185307;
+
+const float 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Not trying to be difficult here - I have attempted to implement the 
straight-forward check.

Added this to the MagicNumbersCheck::MagicNumbersCheck constructor:

  +  // process set of ignored floating point values
  +  const std::vector IgnoredFloatingPointValuesInput =
  +  utils::options::parseStringList(Options.get(
  +  "IgnoredFloatingPointValues", DefaultIgnoredFloatingPointValues));
  +  IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
  +  for (const auto  : IgnoredFloatingPointValuesInput) {
  +llvm::APFloat FloatValue(llvm::APFloat::IEEEdouble());
  +FloatValue.convertFromString(InputValue, DefaultRoundingMode);
  +const double Value = FloatValue.convertToDouble();
  +IgnoredFloatingPointValues.push_back(Value);
  +  }
  +  llvm::sort(IgnoredFloatingPointValues.begin(),
  + IgnoredFloatingPointValues.end());

and changed isIgnoredValue(const FloatingLiteral *Literal) like this:

   bool MagicNumbersCheck::isIgnoredValue(const FloatingLiteral *Literal) const 
{
  -  llvm::APFloat FloatValue = Literal->getValue();
  -  return FloatValue.isZero();
  +  const llvm::APFloat FloatValue = Literal->getValue();
  +  double Value;
  +  if (() == ::APFloat::IEEEdouble()) {
  +Value = FloatValue.convertToDouble();
  +  } else if (() == ::APFloat::IEEEsingle()) {
  +Value = static_cast(FloatValue.convertToFloat());
  +  } else
  +return false;
  +
  +  return std::binary_search(IgnoredFloatingPointValues.begin(),
  +IgnoredFloatingPointValues.end(), Value);
   }

When running under the debugger and printing various values, it seems that 3.14 
read as double and converted to double prints as 3.1397, while 
3.14f read as float and cast to double prints as 3.141049041748

I will parse the IgnoredFloatingPointValues twice and store the results in two 
arrays, one of doubles and one of floats.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.

aaron.ballman wrote:
> 0x8000- wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > 0x8000- wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > I am curious to know how true this is. You got some data 
> > > > > > > > > > for integer values and reported it, but I'm wondering if 
> > > > > > > > > > you've tried the same experiment with floating-point 
> > > > > > > > > > numbers?
> > > > > > > > > The problem with the floating point numbers as text is: they 
> > > > > > > > > need to be parsed both from the configuration and from the 
> > > > > > > > > source code _then_ compared. What is an acceptable epsilon? I 
> > > > > > > > > don't know. Is the same epsilon acceptable on all source 
> > > > > > > > > code? I don't know.
> > > > > > > > Yeah, I'm not too worried about the situations in which the 
> > > > > > > > epsilon matters. I'm more worried that we'll see a lot of 1.0, 
> > > > > > > > 2.0 floating-point literals where the floating-point value is a 
> > > > > > > > nice, round, easy-to-represent number but users have no way to 
> > > > > > > > disable this diagnostic short of `const float Two = 2.0f;`
> > > > > > > Random thought: the types that are ignored should/could be 
> > > > > > > configurable, i.e. there should be a switch
> > > > > > > whether or not to complain about floats.
> > > > > > Even though they might be nice and round... they should mean 
> > > > > > _something_ other than 'Two'.
> > > > > > 
> > > > > > The thing is... magic integers are used as buffer sizes, or to map 
> > > > > > things that are discrete in nature - number of legs of a typical 
> > > > > > mammal for instance. Not sure what magic numbers exist in nature 
> > > > > > besides pi and e and some fundamental physical constants 
> > > > > > )Avogadro's number, etc). But even there, it is better to use a 
> > > > > > symbolic constant.
> > > > > Actually that is a _great_ idea, thank you!
> > > > > The thing is... magic integers are used as buffer sizes, or to map 
> > > > > things that are discrete in nature - number of legs of a typical 
> > > > > mammal for instance. Not sure what magic numbers exist in nature 
> > > > > besides pi and e and some fundamental physical constants )Avogadro's 
> > > > > number, etc). But even there, it is better to use a symbolic constant.
> > > > 
> > > > That's my point -- I think there's a lot of uses of round 
> > > > floating-point values that are not magical numbers, they're sensible 
> > > > constants. Looking at LLVM's code base shows a *lot* of 1.0 and 2.0 
> > > > values (hundreds of instances from a quick text-based search). No one 
> > > > should be forced to turn those into named constants. However, I've seen 
> > > > code using `1.02` and `.98` in places -- those seem like sensible 
> > > > things to make named constants because the values have semantically 
> > > > interesting meaning to the surrounding code.
> > > > 
> > > > > Random thought: the types that are ignored should/could be 
> > > > > configurable, i.e. there should be a switch
> > > > whether or not to complain about floats.
> > > > 
> > > > I think this would be a useful option, for sure (I used to work at a 
> > > > place that did a ton of floating-point math that would benefit from the 
> > > > integer side of this check but could never use the floating-point side 
> > > > of it). However, the presence of such an option doesn't give us a pass 
> > > > on coming up with a data-driven list of default values to ignore for 
> > > > the floating-point side. If we don't want to make that list 
> > > > configurable, I think that's something we can discuss (I think I'm fine 
> > > > with not making it a user-facing configuration option). But I think 
> > > > that `0.0` is insufficient.
> > > Yep. I didn't mean for that flag to be a replacement for the ignore-list 
> > > for fp constants.
> > This opens up an entire can of worms that requires quite a bit of thought 
> > and invites significant amount of bikesheding. Is "2.00" as acceptable as 
> > "2.0"? Do we compare textually, or with regular expressions? Is 2.0001 
> > represented the same way on PowerPC and ARM as on Intel?
> > 
> > As this check is specified and implemented right now, people have the 
> > following options:
> > * not use the check at all
> > * use it to flag all literals
> > * use it to flag only integral literals
> > * use it to flag all integral literals not on a white list.
> > 
> > If somebody can come up with a more logical spec for how to filter out 
> > certain floating 

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added inline comments.



Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.

lebedev.ri wrote:
> aaron.ballman wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > lebedev.ri wrote:
> > > > > aaron.ballman wrote:
> > > > > > 0x8000- wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > I am curious to know how true this is. You got some data for 
> > > > > > > > integer values and reported it, but I'm wondering if you've 
> > > > > > > > tried the same experiment with floating-point numbers?
> > > > > > > The problem with the floating point numbers as text is: they need 
> > > > > > > to be parsed both from the configuration and from the source code 
> > > > > > > _then_ compared. What is an acceptable epsilon? I don't know. Is 
> > > > > > > the same epsilon acceptable on all source code? I don't know.
> > > > > > Yeah, I'm not too worried about the situations in which the epsilon 
> > > > > > matters. I'm more worried that we'll see a lot of 1.0, 2.0 
> > > > > > floating-point literals where the floating-point value is a nice, 
> > > > > > round, easy-to-represent number but users have no way to disable 
> > > > > > this diagnostic short of `const float Two = 2.0f;`
> > > > > Random thought: the types that are ignored should/could be 
> > > > > configurable, i.e. there should be a switch
> > > > > whether or not to complain about floats.
> > > > Even though they might be nice and round... they should mean 
> > > > _something_ other than 'Two'.
> > > > 
> > > > The thing is... magic integers are used as buffer sizes, or to map 
> > > > things that are discrete in nature - number of legs of a typical mammal 
> > > > for instance. Not sure what magic numbers exist in nature besides pi 
> > > > and e and some fundamental physical constants )Avogadro's number, etc). 
> > > > But even there, it is better to use a symbolic constant.
> > > Actually that is a _great_ idea, thank you!
> > > The thing is... magic integers are used as buffer sizes, or to map things 
> > > that are discrete in nature - number of legs of a typical mammal for 
> > > instance. Not sure what magic numbers exist in nature besides pi and e 
> > > and some fundamental physical constants )Avogadro's number, etc). But 
> > > even there, it is better to use a symbolic constant.
> > 
> > That's my point -- I think there's a lot of uses of round floating-point 
> > values that are not magical numbers, they're sensible constants. Looking at 
> > LLVM's code base shows a *lot* of 1.0 and 2.0 values (hundreds of instances 
> > from a quick text-based search). No one should be forced to turn those into 
> > named constants. However, I've seen code using `1.02` and `.98` in places 
> > -- those seem like sensible things to make named constants because the 
> > values have semantically interesting meaning to the surrounding code.
> > 
> > > Random thought: the types that are ignored should/could be configurable, 
> > > i.e. there should be a switch
> > whether or not to complain about floats.
> > 
> > I think this would be a useful option, for sure (I used to work at a place 
> > that did a ton of floating-point math that would benefit from the integer 
> > side of this check but could never use the floating-point side of it). 
> > However, the presence of such an option doesn't give us a pass on coming up 
> > with a data-driven list of default values to ignore for the floating-point 
> > side. If we don't want to make that list configurable, I think that's 
> > something we can discuss (I think I'm fine with not making it a user-facing 
> > configuration option). But I think that `0.0` is insufficient.
> Yep. I didn't mean for that flag to be a replacement for the ignore-list for 
> fp constants.
This opens up an entire can of worms that requires quite a bit of thought and 
invites significant amount of bikesheding. Is "2.00" as acceptable as "2.0"? Do 
we compare textually, or with regular expressions? Is 2.0001 represented 
the same way on PowerPC and ARM as on Intel?

As this check is specified and implemented right now, people have the following 
options:
* not use the check at all
* use it to flag all literals
* use it to flag only integral literals
* use it to flag all integral literals not on a white list.

If somebody can come up with a more logical spec for how to filter out certain 
floating point values - they can definitely extend this check, without 
impacting users that are happy with the menu offered above.

Does this sound reasonable?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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