[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL closed this revision.
PiotrZSL added a comment.

Obsolete. No longer needed after D159436 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

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

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In D159045#4631820 , @felix642 wrote:

> In that case, I agree with you, it would be helpful to add this feature. I 
> think supporting an empty value rather than a boolean is preferable. We 
> should maybe do that in another Differential though. I can open an issue on 
> github and I'll open another diff when I'm ready. What do you think?

Yes this should be done as a separate change.
Probably something like this would be needed in ClangTidyCheck.h.

  template 
  std::enable_if_t, std::optional> get(StringRef 
LocalName, std::optional Default) const {
if (std::optional Value = get(LocalName)) {
  if (value == "none" || value == "null" || value == "false" || value 
== "") // and -1 for unsigned types for backward combability
return std::nullopt;
  T Result{};
  if (!StringRef(*Value).getAsInteger(10, Result))
return Result;
  diagnoseBadIntegerOption(NamePrefix + LocalName, *Value);
}
return Default;
   }

And storing would need to be updated in similar way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

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

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 added a comment.

In that case, I agree with you, it would be helpful to add this feature. I 
think supporting an empty value rather than a boolean is preferable. We should 
maybe do that in another Differential though. I can open an issue on github and 
I'll open another diff when I'm ready. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

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

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In D159045#4631650 , @felix642 wrote:

> We can already disable those options if we don't define them in the config. 
> Adding the possibility to provide optional values seems redundant to me. Do 
> you see any reason why we would absolutely need to add this option to the 
> config if we want to disable it?

a) If you use config file inheritance, when one config file define it, and 
other that inherit it (or command line) want to disable such option.
b) When we use --dump-config then it will be printed anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

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

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 added a comment.

We can already disable those options if we don't define them in the config. 
Adding the possibility to provide optional values seems redundant to me. Do you 
see any reason why we would absolutely need to add this option to the config if 
we want to disable it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

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

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Piotr Zegar via Phabricator via cfe-commits]

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

Change is ok. Interesting thing is that we got more of such issues in many 
checks. I'm thinking, maybe support for optional fields is needed, fields that 
can accept integer or false/no values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

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

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 updated this revision to Diff 554124.
felix642 added a comment.

Linked issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

Files:
  clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst


Index: clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
@@ -12,8 +12,8 @@
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This option is disabled by 
+   default.
 
 .. option:: StatementThreshold
 
@@ -23,23 +23,23 @@
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This option is 
+   disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This option 
+   is disabled by default.
 
 .. option:: NestingThreshold
 
 Flag compound statements which create next nesting level after
 `NestingThreshold`. This may differ significantly from the expected value
-for macro-heavy code. The default is `-1` (ignore the nesting level).
+for macro-heavy code. This option is disabled by default.
 
 .. option:: VariableThreshold
 
Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
Please note that function parameters and variables declared in lambdas,
-   GNU Statement Expressions, and nested class inline functions are not 
counted.
+   GNU Statement Expressions, and nested class inline functions are not 
+   counted. This option is disabled by default.


Index: clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
@@ -12,8 +12,8 @@
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This option is disabled by 
+   default.
 
 .. option:: StatementThreshold
 
@@ -23,23 +23,23 @@
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This option is 
+   disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This option 
+   is disabled by default.
 
 .. option:: NestingThreshold
 
 Flag compound statements which create next nesting level after
 `NestingThreshold`. This may differ significantly from the expected value
-for macro-heavy code. The default is `-1` (ignore the nesting level).
+for macro-heavy code. This option is disabled by default.
 
 .. option:: VariableThreshold
 
Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
Please note that function parameters and variables declared in lambdas,
-   GNU Statement Expressions, and nested class inline functions are not counted.
+   GNU Statement Expressions, and nested class inline functions are not 
+   counted. This option is disabled by default.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 updated this revision to Diff 554122.
felix642 added a comment.

Fixed commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159045

Files:
  clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst


Index: clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
@@ -12,8 +12,8 @@
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This option is disabled by 
+   default.
 
 .. option:: StatementThreshold
 
@@ -23,23 +23,23 @@
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This option is 
+   disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This option 
+   is disabled by default.
 
 .. option:: NestingThreshold
 
 Flag compound statements which create next nesting level after
 `NestingThreshold`. This may differ significantly from the expected value
-for macro-heavy code. The default is `-1` (ignore the nesting level).
+for macro-heavy code. This option is disabled by default.
 
 .. option:: VariableThreshold
 
Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
Please note that function parameters and variables declared in lambdas,
-   GNU Statement Expressions, and nested class inline functions are not 
counted.
+   GNU Statement Expressions, and nested class inline functions are not 
+   counted. This option is disabled by default.


Index: clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
@@ -12,8 +12,8 @@
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This option is disabled by 
+   default.
 
 .. option:: StatementThreshold
 
@@ -23,23 +23,23 @@
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This option is 
+   disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This option 
+   is disabled by default.
 
 .. option:: NestingThreshold
 
 Flag compound statements which create next nesting level after
 `NestingThreshold`. This may differ significantly from the expected value
-for macro-heavy code. The default is `-1` (ignore the nesting level).
+for macro-heavy code. This option is disabled by default.
 
 .. option:: VariableThreshold
 
Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
Please note that function parameters and variables declared in lambdas,
-   GNU Statement Expressions, and nested class inline functions are not counted.
+   GNU Statement Expressions, and nested class inline functions are not 
+   counted. This option is disabled by default.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159045: [clang-tidy] Improved documentation for readability-function-size

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 created this revision.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
felix642 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The documentation would say that that default value for most parameters is -1. 
But since the parameter used in clang-tidy is an unsigned the value would get 
implicitly converted to 4294967295.

If a user tried to use -1 to disable this check he would receive an error 
saying that
the parameter is invalid.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159045

Files:
  clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst


Index: clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
@@ -12,8 +12,8 @@
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This option is disabled by 
+   default.
 
 .. option:: StatementThreshold
 
@@ -23,23 +23,23 @@
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This option is 
+   disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This option 
+   is disabled by default.
 
 .. option:: NestingThreshold
 
 Flag compound statements which create next nesting level after
 `NestingThreshold`. This may differ significantly from the expected value
-for macro-heavy code. The default is `-1` (ignore the nesting level).
+for macro-heavy code. This option is disabled by default.
 
 .. option:: VariableThreshold
 
Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
Please note that function parameters and variables declared in lambdas,
-   GNU Statement Expressions, and nested class inline functions are not 
counted.
+   GNU Statement Expressions, and nested class inline functions are not 
+   counted. This option is disabled by default.


Index: clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
@@ -12,8 +12,8 @@
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This option is disabled by 
+   default.
 
 .. option:: StatementThreshold
 
@@ -23,23 +23,23 @@
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This option is 
+   disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This option 
+   is disabled by default.
 
 .. option:: NestingThreshold
 
 Flag compound statements which create next nesting level after
 `NestingThreshold`. This may differ significantly from the expected value
-for macro-heavy code. The default is `-1` (ignore the nesting level).
+for macro-heavy code. This option is disabled by default.
 
 .. option:: VariableThreshold
 
Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
Please note that function parameters and variables declared in lambdas,
-   GNU Statement Expressions, and nested class inline functions are not counted.
+   GNU Statement Expressions, and nested class inline functions are not 
+   counted. This option is disabled by default.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits