[PATCH] D154287: [clang-tidy] Add modernize-use-std-format check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:168
 
+- New :doc:`modernize-use-std-format
+  ` check.

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154287

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

[PATCH] D158480: [clang-tidy][readability-braces-around-statements] ignore false-positive for constexpr if statement in lambda expression

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:233
+  ` check to
+  ignore false-positive for constexpr if statement in lambda expression.
+

`if constexpr`? Please also enclose it in double back-ticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158480

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

[PATCH] D158244: [clang-tidy]`pro-bounds-array-to-pointer-decay` ignore predefined expression

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:191
+  ` 
check 
+  to ignore predefined expression (e.g., __func__, ...).
+

Please enclose `__func__` in double back-ticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158244

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

[PATCH] D158152: [clang-tidy]mark record initList as non-const param

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:234
 
+- Improved :doc:`readability-non-const-parameter.cpp
+  ` check to ignore

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158152

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

[PATCH] D157326: [clang-tidy] Fix handling of out-of-line functions in readability-static-accessed-through-instance

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
 static unsigned getNameSpecifierNestingLevel(const QualType ) {
   if (const ElaboratedType *ElType = QType->getAs()) {
 if (const NestedNameSpecifier *NestedSpecifiers = ElType->getQualifier()) {

Should be `const auto *`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157326

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

[PATCH] D157239: [clang-tidy] Implement bugprone-incorrect-enable-if

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst:6
+
+Detects incorrect usages of std::enable_if that don't name the nested 'type'
+type.

Please synchronize with statement in Release Notes after Piotr's comment 
addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157239

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

[PATCH] D157188: [clang-tidy] Add bugprone-allocation-bool-conversion

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp:25
+  "malloc;calloc;realloc;strdup;fopen;fdopen;freopen;"
+  "opendir;fdopendir;popen;mmap;allocate"))) {}
+

POSIX `open`, `openat`, `creat`, `pthread_create` should be added too.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp:60
+ "result of the 'new' expression is being used as a boolean value, "
+ "which "
+ "may lead to unintended behavior or resource leaks");

Should be merged together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157188

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

[PATCH] D157188: [clang-tidy] Add bugprone-new-bool-conversion

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D157188#4563139 , @PiotrZSL wrote:

> In D157188#4563105 , 
> @Eugene.Zelenko wrote:
>
>> Shouldn't C-style allocation be checked too? Same for custom allocators via 
>> configuration option.
>
> In theory it could, but in such case name of check would need to change...

Something like `bugprone-resource-bool-conversion` should be generic enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157188

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

[PATCH] D157188: [clang-tidy] Add bugprone-new-bool-conversion

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Same for file, threads and other resources allocation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157188

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

[PATCH] D157188: [clang-tidy] Add bugprone-new-bool-conversion

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Shouldn't C-style allocation be checked too? Same for custom allocators via 
configuration option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157188

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

[PATCH] D157057: [clang-tidy] Implement cppcoreguidelines CP.52

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:156
 
+- New :doc:`cppcoreguidelines-no-suspend-with-lock
+  ` check.

Please keep alphabetical order in new checks list.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock.rst:6
+
+Warns when a coroutine suspends while a lock guard object is in scope.
+

Please synchronize with statement in Release Notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157057

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D144135#4524902 , @PiotrZSL wrote:

> @Eugene.Zelenko I cannot review/accept my own patch :(.

I'm sorry for noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144135

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

[PATCH] D154999: [clang-tidy] Add bugprone-std-forward-type-mismatch check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst:28
+is a mismatch between the type passed as an argument and the template 
parameter.
+

Excessive newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154999

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

[PATCH] D154287: [clang-tidy] Add modernize-use-std-format check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:6
+
+Converts calls to ``absl::StrFormat`` to equivalent calls to C++20's
+``std::format`` function, modifying the format string appropriately. The

Please synchronize first statement with Release Notes.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:58
+  of -42 and the signed representation of 0x (often 4294967254
+  and -1 respectively.) When false (which is the default), these casts
+  will not be added which may cause a change in the output. Note that this

Please highlight `false` with single back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:70
+   immediately afterwards. The defaualt value for this option is
+   ``absl::StrFormat``.
+

Single back-ticks for option value.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:84
+   `ReplacementFormatFunction` so that a ``#include`` directive can be added if
+   required. If `ReplacementFormatFunction` is ``std::format`` then this 
option will
+   default to , otherwise this option will default to nothing

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154287

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

[PATCH] D154297: clang-tidy: accessing checks not done for aliases of `std::array`

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention this change in Release Notes.


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

https://reviews.llvm.org/D154297

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

[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D149280#4446591 , @mikecrowe wrote:

> @Eugene.Zelenko,
> I will make the documentation changes you've suggested, but I can't say I 
> really understand which document elements require single backticks and which 
> require dual backticks. https://llvm.org/docs/SphinxQuickstartTemplate.html 
> seems to imply that single backticks are only used for links and double 
> backticks are used for monospace, but it's not completely clear. Most of the 
> existing documentation seems to be consistent with what you've suggested 
> though.

In short: double back-ticks are for language constructs. Single - for 
command-line options, options and their values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

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

[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:29
+- It assumes that the format string is correct for the arguments. If you
+  get any warnings when compiling with ``-Wformat`` then misbehaviour is
+  possible.

Please use single back-ticks for `-Wformat` (command-line option).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:81
+
+   When true, the check will add casts when converting from variadic
+   functions like ``printf`` and printing signed or unsigned integer types

Please highlight `true` with back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:87
+   converting from non-variadic functions such as ``absl::PrintF`` and
+   ``fmt::printf``. For example, with ``StrictMode`` enabled:
+

Please use single back-ticks for `StrictMode` (option).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:103
+  of -42 and the signed representation of 0x (often 4294967254
+  and -1 respectively.) When false (which is the default), these casts
+  will not be added which may cause a change in the output.

Please highlight `false` and numbers with back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:112
+   immediately afterwards. If neither this option nor
+   ``FprintfLikeFunctions`` are set then the default value for this option
+   is ``printf; absl::PrintF``, otherwise it is empty.

Please use single back-ticks for `FprintfLikeFunctions` (option). Same for 
`printf; absl::PrintF` below.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:122
+   arguments to be formatted follow immediately afterwards. If neither this
+   option nor ``PrintfLikeFunctions`` are set then the default value for
+   this option is ``fprintf; absl::FPrintF``, otherwise it is empty.

Ditto.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:144
+   The header that must be included for the declaration of
+   ``ReplacementPrintFunction`` so that a ``#include`` directive can be
+   added if required. If ``ReplacementPrintFunction`` is ``std::print``

Please use single back-ticks for `ReplacementPrintFunction` (option).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

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

[PATCH] D153423: [clang-tidy] Allow explicit throwing in bugprone-exception-escape for special functions

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst:29
+and destructors, it is a clear indication of the developer's intention and
+should be respected..
+

Double period.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153423

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

[PATCH] D152589: Add readability test for not allowing relative includes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/AbsoluteIncludesOnlyCheck.cpp:16
+
+
+namespace clang::tidy::readability {

Excessive newline.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:187
+
+Finds relative includes in your code and warn about them. for example
+don't use "readability/absolute-includes-only", use 


Please make it as first statement in documentation.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst:8
+
+Meaning this check disallow the use of quote includes (""), and allow only 
angular brackets includes (<>).
+

Please highlight `""` and `<>` with double back-ticks. Please follow 
80-characters limit.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst:17
+
+  // #include "utility.hpp"   // Wrong.
+  // #include// Wrong.

Please split example on right and wrong blocks.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability/absolute-includes-only.cpp:14
+
+
+#include 

Excessive newline.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability/absolute-includes-only.cpp:18
+#include 
+

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152589

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

[PATCH] D148793: [clang-tidy] Implement an include-cleaner check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:186
 
+- New :doc:`misc-include-cleaner
+  ` check.

Please keep alphabetical order (by check name) in this section.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:189
+
+  Checks for unused and missing includes. Generates findings only for
+  the main file of a translation unit.

One sentence is enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793

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

[PATCH] D151383: [clang-tidy] Check for specific return types on all functions

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:398
 
+
 Removed checks

Excessive newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151383

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

[PATCH] D151383: [clang-tidy] Check for specific return types on all functions

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:394
 
+- Extend ``bugprone-unused-return-value`` check to check for all functions
+  with specified return types using the ``CheckedReturnTypes`` option.

Please keep alphabetical order in section (by check name).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-return-value.rst:53
+   By default the following function return types are checked:
+   ``std::error_code std::expected boost::system::error_code abseil::Status``
+

Please use single back-tick for option values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151383

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

[PATCH] D151383: [clang-tidy] Check for specific return types on all functions

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please extend test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151383

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

[PATCH] D151092: [clang-tidy]performance-no-automatic-move: fix false negative on `const T&&` ctors.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:385
+- Improved :doc:`performance-no-automatic-move
+  `: warn on 
`const &&`
+  constructors.

Please use double back-ticks for `const &&`.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp:118
 }
+

Is it really needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151092

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

[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
 
+- New :doc:`cppcoreguidelines-missing-std-forward
+  ` check.

Should be after `cppcoreguidelines-misleading-capture-default-by-value`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146921

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

[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+

mikecrowe wrote:
> Eugene.Zelenko wrote:
> > Please add information about default value.
> The current implementation always "fixes" `printf`, `fprintf`, `absl::PrintF` 
> and `absl::FPrintf`, even when this option is used (see `UseStdPrintCheck` 
> constructor.) Should the option inhibit this default?
It may be reasonable to provide possibility to override default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

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

[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:66
+  if (Converter.canApply()) {
+const auto *PrintfCall = Printf->getCallee();
+DiagnosticBuilder Diag =

Please don't use `auto` unless type is explicitly stated in same statement or 
iterator.



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.h:15
+namespace clang::tidy::modernize {
+/// Documentation goes here.
+///

Should be elaborated or removed.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:171
 
+- New :doc: `modernize-printf-to-std-print
+  ` check.

Please keep alphabetical order (by check name) in this section.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:6
+
+This check is capable of converting calls to printf to calls to std::print
+whilst also modifying the format string appropriately.

Please make first statement same as in Release Notes. Please highlight language 
constructs (like `printf`) with double back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:11
+
+ fprintf(stderr, "The %s is %3d\n", answer, value);
+

Please prepend with `.. code-block:: c++` line. Same for other snippets.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+

Please add information about default value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion.rst:43
+safety of the conversion before using an explicit cast.
+This extra level of caution can help catch potential issues early on in the
+development process, improving the overall reliability and maintainability of

Should continue previous line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149084

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

[PATCH] D149088: [clang-format] Add run-clang-format.py script.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang/tools/run-clang-format.py:1
+#!/usr/bin/env python3
+#

Looks like Windows en of lines were mixed with UNIX ones.



Comment at: clang/tools/run-clang-format.py:26
+
+from __future__ import print_function
+import argparse

Is it really necessary since Python 3 is specified explicitly?



Comment at: clang/tools/run-clang-format.py:30
+import os
+import multiprocessing
+import queue

Should be before `os`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149088

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

[PATCH] D148793: [WIP][clang-tidy] Implement an include-cleaner check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D148793#4283733 , @carlosgalvezp 
wrote:

> I believe this should be discussed in an RFC. We already have the standalone 
> `include-cleaner` tool, why is that not sufficient? Can it be extended 
> instead? There's also the include-what-you-use 
>  tool out there.

On my understanding, `include-cleaner` is re-implementation of `IWYU`. Awhile 
ago `IWYU` maintainers tried to merge their project to LLVM. Discussion should 
be in old mailing lists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793

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

[PATCH] D148793: [clang-tidy] Implement an include-cleaner check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Is there real advantage in Clang-tidy check versus standalone tool?




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:144
+
+  FIXME: add release notes.
+

Please add one sentence description.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/google/include-cleaner.rst:6
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

Missing documentation and may be examples.



Comment at: clang-tools-extra/test/clang-tidy/checkers/google/Inputs/bar.h:5
+int bar();
\ No newline at end of file


Please add. Same in other files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793

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

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko accepted this revision.
Eugene.Zelenko added a comment.

Please create issue for deprecation after commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148460

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

[PATCH] D148340: [clang-tidy] Apply cppcoreguidelines-avoid-capture-default-when-capturin-this only to by-value capture default

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h:1
+//===--- MisleadingCaptureDefaultByValueCheck.h - clang-tidy*- C++
+//-*-===//

Split header comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148340

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

[PATCH] D148340: [clang-tidy] Apply cppcoreguidelines-avoid-capture-default-when-capturin-this only to by-value capture default

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidByValueCaptureDefaultWhenCapturingThisCheck.h:1
-//===--- AvoidCaptureDefaultWhenCapturingThisCheck.h - clang-tidy*- C++ 
-*-===//
+//===--- AvoidByValueCaptureDefaultWhenCapturingThisCheck.h - clang-tidy*- C++
+//-*-===//

Please make it single line by removing all possible `=` and `-`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148340

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

[PATCH] D147876: [clang-tidy] Support introducing checks as a list in the config file

[Eugene Zelenko via Phabricator via cfe-commits]

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

Looks OK for me but will be good idea to wait for other reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147876

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

[PATCH] D137782: [clang-tidy]bugprone-fold-init-type

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please rebase from latest  `main`.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:294
 
+- Improved :doc:`bugprone-fold-init-type
+  ` to handle iterators that do not

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137782

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

[PATCH] D147563: [clang-tidy] Deprecate cert-dcl21-cpp

[Eugene Zelenko via Phabricator via cfe-commits]

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

I think will be good idea to track deprecated features on GitHub issues with 
something like `deprecate in XYZ` label(s).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147563

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

[PATCH] D147379: [clang-tidy] Disable misc-definitions-in-headers for declarations in anonymous namespaces

[Eugene Zelenko via Phabricator via cfe-commits]

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

Looks OK for me, but will be good idea to wait for other reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147379

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst:46
+expressions that match the methods.
+Default value is `"::value$;::get$`.

Unintended quote in value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147357

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

[PATCH] D147315: [clang-tidy] support unscoped enumerations in readability-static-accessed-through-instance

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:264
 
+- Improved :doc:`readability-static-accessed-through-instance
+  ` check to 

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147315

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

[PATCH] D146922: [clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:260
 
+- Fixed an issue in :doc:`performance-noexcept-move-constructor
+  ` which resulted in 
false

AMS21 wrote:
> Eugene.Zelenko wrote:
> > Please keep alphabetical order (by check name) in this section.
> Sure but I'm a bit confused. I assuming the check name is without the prefix 
> so in this case `noexcept-move-constructor`. So it should be after 
> `readability-avoid-underscore-in-googletest-name`. But why then is 
> `use-after-move` sorted before `identifier-naming`. 
> I'm probably missing something very obvious.
Full check name (i.e. `performance-noexcept-move-constructor`) should be 
accounted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146922

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

[PATCH] D146922: [clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:260
 
+- Fixed an issue in :doc:`performance-noexcept-move-constructor
+  ` which resulted in 
false

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146922

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

[PATCH] D146922: [clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:83
+   DefaultableMemberKind Kind) {
+  auto *RecType = Base.getType()->getAs();
+  if (!RecType)

Should be `const auto *`.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:87
+
+  auto *BaseClass = cast(RecType->getDecl());
+

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146922

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

[PATCH] D147081: [clang-tidy] Fix issues in bugprone-assert-side-effect

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp:66
+
+  const auto *FuncDecl = CExpr->getDirectCallee();
+  if (!FuncDecl || !FuncDecl->getDeclName().isIdentifier())

PiotrZSL wrote:
> Eugene.Zelenko wrote:
> > Please do not use `auto` if type is not spelled explicitly or iterator.
> That `auto` already was there, I just moved it... :)
So it's great reason to finally fix it :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147081

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

[PATCH] D147081: [clang-tidy] Fix issues in bugprone-assert-side-effect

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp:66
+
+  const auto *FuncDecl = CExpr->getDirectCallee();
+  if (!FuncDecl || !FuncDecl->getDeclName().isIdentifier())

Please do not use `auto` if type is not spelled explicitly or iterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147081

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

[PATCH] D145581: [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:222
 
-- Improved :doc:`bugprone-use-after-move
-  ` to understand that there is a
-  sequence point between designated initializers.
+- In :doc:`bugprone-use-after-move
+  `:

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145581

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

[PATCH] D146947: [docs][clang] Added extra information inside the flag -fsanitize=unsigned-shift-base for UndefinedBehaviorSanitizer Fix : #60712

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:160
   -  ``-fsanitize=unsigned-shift-base``: check that an unsigned left-hand side 
of
- a left shift operation doesn't overflow.
+ a left shift operation doesn't overflow.Issues caught by this sanitizer 
are 
+ not undefined behavior, but are often unintentional.

Missing space after period.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146947

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

[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:127
+
+  Warns when a forwarding reference parameter is not forwarded within the 
function body.
+

Please follow 80 characters limit for text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146921

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

[PATCH] D146922: [clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp:64
 // CHECK-FIXES: ){{.*}}noexcept{{.*}} {}
\ No newline at end of file


Please add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146922

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

[PATCH] D146904: [clang-tidy] Fix extern fixes in readability-redundant-declaration

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantDeclarationCheck.cpp:84
+  SourceLocation BeginLoc;
+  if (const LinkageSpecDecl *Extern =
+  Result.Nodes.getNodeAs("extern");

`auto` could be used because type is explicitly stated in same statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146904

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

[PATCH] D145617: [clang-tidy] Add readability-avoid-unconditional-preprocessor-if check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139
+
+  Check flags always enabled or disabled code blocks in preprocessor ``#if``
+  conditions, such as ``#if 0`` and ``#if 1`` etc.

Please omit `Check`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst:6
+
+Check flags always enabled or disabled code blocks in preprocessor ``#if``
+conditions, such as ``#if 0`` and ``#if 1`` etc.

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145617

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

[PATCH] D144522: [clang-tidy] Add readability-operators-representation check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139
+
+  Check helps enforce consistent token representation for invoked binary,
+  unary and overloaded operators in C++ code.

Please omit `Check`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/operators-representation.rst:6
+
+Check helps enforce consistent token representation for invoked binary, unary
+and overloaded operators in C++ code. The check supports both traditional and

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144522

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

[PATCH] D146655: [clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Fixed an issue in :doc:`google-avoid-underscore-in-googletest-name
+  ` when using

carlosgalvezp wrote:
> Eugene.Zelenko wrote:
> > carlosgalvezp wrote:
> > > Eugene.Zelenko wrote:
> > > > Eugene.Zelenko wrote:
> > > > > carlosgalvezp wrote:
> > > > > > PiotrZSL wrote:
> > > > > > > carlosgalvezp wrote:
> > > > > > > > Eugene.Zelenko wrote:
> > > > > > > > > Please keep alphabetical order (by check name) in this 
> > > > > > > > > section.
> > > > > > > > I was planning to do that but noticed that the alphabetical 
> > > > > > > > order is already broken. It seems to be a source of friction 
> > > > > > > > and there's no official documentation that states it should be 
> > > > > > > > done like that, so I can understand if it gets broken often. Do 
> > > > > > > > you know if this is documented somewhere? If not, do we see 
> > > > > > > > value in keeping this convention? I suppose now we would need 
> > > > > > > > an NFC patch to fix the order again, causing churn.
> > > > > > > I run into same issue also. I would say, let leave it as it is, 
> > > > > > > and fix it with one commit at the end of release.
> > > > > > Good idea, let's do that!
> > > > > Often it's also broken after rebases which may be automatic.
> > > > Anyway, some kind of order is much better than disorder.
> > > Definitely. Could we stick to some simple convention? For example always 
> > > append or prepend to the list of modifications to checks. Then before 
> > > release we put up a patch for reordering.
> > I think it will be harder to reader. Sorting by check name is much better 
> > in this respect. And this was used in many releases.
> To clarify, what I mean is:
> 
> - Apply a simple convention (e.g. append or prepend to the list) //while 
> developing//.
> - Right before creating a release, put up a patch to sort alphabetically. 
> Then it will be easy to read for users when it's released.
> 
> Or do you mean that the list shall be alphabetically sorted at all times?
It'll be much easier to sort partially sorted list at release time than 
completely unsorted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146655

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

[PATCH] D146655: [clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Fixed an issue in :doc:`google-avoid-underscore-in-googletest-name
+  ` when using

carlosgalvezp wrote:
> Eugene.Zelenko wrote:
> > Eugene.Zelenko wrote:
> > > carlosgalvezp wrote:
> > > > PiotrZSL wrote:
> > > > > carlosgalvezp wrote:
> > > > > > Eugene.Zelenko wrote:
> > > > > > > Please keep alphabetical order (by check name) in this section.
> > > > > > I was planning to do that but noticed that the alphabetical order 
> > > > > > is already broken. It seems to be a source of friction and there's 
> > > > > > no official documentation that states it should be done like that, 
> > > > > > so I can understand if it gets broken often. Do you know if this is 
> > > > > > documented somewhere? If not, do we see value in keeping this 
> > > > > > convention? I suppose now we would need an NFC patch to fix the 
> > > > > > order again, causing churn.
> > > > > I run into same issue also. I would say, let leave it as it is, and 
> > > > > fix it with one commit at the end of release.
> > > > Good idea, let's do that!
> > > Often it's also broken after rebases which may be automatic.
> > Anyway, some kind of order is much better than disorder.
> Definitely. Could we stick to some simple convention? For example always 
> append or prepend to the list of modifications to checks. Then before release 
> we put up a patch for reordering.
I think it will be harder to reader. Sorting by check name is much better in 
this respect. And this was used in many releases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146655

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

[PATCH] D146712: [clang-tidy] Add portability-non-portable-integer-constant check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please add alias entry in Release Notes.




Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:95
+
+
+void NonPortableIntegerConstantCheck::check(

Excessive newline.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
 
+- New :doc:`portability-non-portable-integer-constant
+  ` check.

Incorrect entry.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/portability/non-portable-integer-constant.rst:4
+portability-non-portable-integer-constant
+==
+

Please make is same length as title.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146712

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

[PATCH] D146655: [clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Fixed an issue in :doc:`google-avoid-underscore-in-googletest-name
+  ` when using

Eugene.Zelenko wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > carlosgalvezp wrote:
> > > > Eugene.Zelenko wrote:
> > > > > Please keep alphabetical order (by check name) in this section.
> > > > I was planning to do that but noticed that the alphabetical order is 
> > > > already broken. It seems to be a source of friction and there's no 
> > > > official documentation that states it should be done like that, so I 
> > > > can understand if it gets broken often. Do you know if this is 
> > > > documented somewhere? If not, do we see value in keeping this 
> > > > convention? I suppose now we would need an NFC patch to fix the order 
> > > > again, causing churn.
> > > I run into same issue also. I would say, let leave it as it is, and fix 
> > > it with one commit at the end of release.
> > Good idea, let's do that!
> Often it's also broken after rebases which may be automatic.
Anyway, some kind of order is much better than disorder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146655

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

[PATCH] D146655: [clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Fixed an issue in :doc:`google-avoid-underscore-in-googletest-name
+  ` when using

carlosgalvezp wrote:
> PiotrZSL wrote:
> > carlosgalvezp wrote:
> > > Eugene.Zelenko wrote:
> > > > Please keep alphabetical order (by check name) in this section.
> > > I was planning to do that but noticed that the alphabetical order is 
> > > already broken. It seems to be a source of friction and there's no 
> > > official documentation that states it should be done like that, so I can 
> > > understand if it gets broken often. Do you know if this is documented 
> > > somewhere? If not, do we see value in keeping this convention? I suppose 
> > > now we would need an NFC patch to fix the order again, causing churn.
> > I run into same issue also. I would say, let leave it as it is, and fix it 
> > with one commit at the end of release.
> Good idea, let's do that!
Often it's also broken after rebases which may be automatic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146655

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

[PATCH] D146713: [clang-tidy][NFC] Improve naming convention in google-readability-avoid-underscore-in-googletest-name

[Eugene Zelenko via Phabricator via cfe-commits]

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

But will be good idea to give a chance to other reviewers to take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146713

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

[PATCH] D146655: [clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Fixed an issue in :doc:`google-avoid-underscore-in-googletest-name
+  ` when using

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146655

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



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

`const auto *`.



Comment at: 
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp:26
+
+auto Self = Node.get();
+if (!Self)

Ditto.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability/reference-to-constructed-temporary.cpp:31
+}
+

Excessive newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146368

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

[PATCH] D146288: clang-tidy: Detect use-after-move in CXXCtorInitializer

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:190
 
+- Improved :doc:`bugprone-use-after-move
+  ` check to also cover constructor

Please keep alphabetical order (by check name) in this section.


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

https://reviews.llvm.org/D146288

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

[PATCH] D145865: [clang-tidy] Multiple fixes to bugprone-exception-escape

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:491
 if (const auto *ThrownExpr = Throw->getSubExpr()) {
-  const auto *ThrownType =
-  ThrownExpr->getType()->getUnqualifiedDesugaredType();
-  if (ThrownType->isReferenceType())
-ThrownType = ThrownType->castAs()
- ->getPointeeType()
- ->getUnqualifiedDesugaredType();
-  Results.registerException(
-  ThrownExpr->getType()->getUnqualifiedDesugaredType());
+  const auto *ThrownType = ThrownExpr->getType()
+   .getCanonicalType()

Please don't use `auto` unless type is explicitly stated in same statement or 
iterator.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:514
   } else {
-const auto *CaughtType =
-Catch->getCaughtType()->getUnqualifiedDesugaredType();
-if (CaughtType->isReferenceType()) {
-  CaughtType = CaughtType->castAs()
-   ->getPointeeType()
-   ->getUnqualifiedDesugaredType();
-}
-
+const auto *CaughtType = Catch->getCaughtType()
+ .getCanonicalType()

Ditto.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:211
 
+- Improved :doc:`bugprone-exception-escape
+  ` by excluding explicitly

Please keep alphabetical order (by check name) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145865

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

[PATCH] D145720: [clang-tidy] Finish cppcoreguidelines-avoid-capturing-lambda-coroutines check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:126
 
-  Adds check for cpp core guideline: "CP.51: Do not use capturing lambdas that
-  are coroutines."
-
+  Check flags C++20 coroutine lambdas with non-empty capture lists that may
+  cause use-after-free errors and suggests avoiding captures or ensuring the

Just `Flags`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst:6
 
-Warns if a capturing lambda is a coroutine. For example:
+Check flags C++20 coroutine lambdas with non-empty capture lists that may cause
+use-after-free errors and suggests avoiding captures or ensuring the lambda

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145720

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

[PATCH] D145617: [clang-tidy] Add readability-avoid-unconditional-preprocessor-if check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp:29
+  return;
+auto  = PP.getSourceManager();
+if (!isStatic(SM, PP.getLangOpts(), ConditionRange))

Please don't use `auto` unless type is spelled explicitly in same statement or 
iterator.



Comment at: 
clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp:43
+SourceRange ConditionRange) {
+
+SourceLocation Loc = ConditionRange.getBegin();

Excessive newline.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:127
+  Check flags always enabled or disabled code blocks in preprocessor ``#if``
+  conditions, such as ``#if 0`` and ``#if 1``.
+

Add `etc.` at the end?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst:7
+Check flags always enabled or disabled code blocks in preprocessor ``#if``
+conditions, such as ``#if 0`` and ``#if 1``.
+

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145617

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

[PATCH] D145581: [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1379
+};
\ No newline at end of file


Please add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145581

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

[PATCH] D145138: [clang-tidy] Implement FixIts for C arrays

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:588
+: ClangTidyCheck(Name, Context),
+  IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+   utils::IncludeSorter::IS_LLVM),

ccotter wrote:
> Eugene.Zelenko wrote:
> > Should be global option, because it's used in other checks.
> Could you clarify this a bit? This is how most other tests consume 
> IncludeStyle (`Options.getLocalOrGlobal("IncludeStyle", 
> utils::IncludeSorter::IS_LLVM)`.
@carlosgalvezp is best person to answer because he recently introduced global 
option for source files and headers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145138

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

[PATCH] D145138: [clang-tidy] Implement FixIts for C arrays

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:94
+
+  auto StartOfFile = SM.getLocForStartOfFile(SM.getFileID(Location));
+  while (Location != StartOfFile) {

Please do not use `auto` unless type is explicitly stated in same statement or 
iterator.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:498
+return false;
+  const CXXConstructExpr *ConstructExpr =
+  dyn_cast(SpelledExpr);

`auto` could be used here.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:588
+: ClangTidyCheck(Name, Context),
+  IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+   utils::IncludeSorter::IS_LLVM),

Should be global option, because it's used in other checks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst:71
 can be either ``char* argv[]`` or ``char** argv``, but cannot be
 ``std::array<>``.

Missing documentation for `IncludeStyle` option if it'll remain local.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145138

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

[PATCH] D142587: [clang-tidy] Improved too-small-loop-variable with bit-field support

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

@carlosgalvezp: Sorry, there are too much Clang specifics in this patch, so I 
could not be reviewer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142587

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst:169
+should be provided. If an exception type name in the list is caught in an
+empty catch statement, no warning will be raised. Default value: `` 
(empty).

PiotrZSL wrote:
> Eugene.Zelenko wrote:
> > Mistake in default value formatting.
> Yy, what mistake ? (empty) not needed, or something else should be used 
> instead of `` ?
Sorry, I mistook (empty) as part of value. May be just say this in words to 
avoid such confusions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144748

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:77
+void EmptyCatchCheck::registerMatchers(MatchFinder *Finder) {
+
+  auto AllowedNamedExceptionDecl =

Excessive newline.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst:169
+should be provided. If an exception type name in the list is caught in an
+empty catch statement, no warning will be raised. Default value: `` 
(empty).

Mistake in default value formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144748

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

[PATCH] D144594: [clang-tidy] Fix bugprone-copy-constructor-init documentation

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/copy-constructor-init.rst:37
+
+In summary check detects cases where the copy constructor of a derived class
+doesn't properly call or initialize the copy constructor of the base class,

English is not my native tongue, but may be `the check` is correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144594

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

[PATCH] D144216: [clang-tidy] Extract string header from redundant-string-cstr checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

May be you could use heard in other test in same patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144216

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

[PATCH] D144037: [clang-tidy] allow tests to use -config-file instead of -config

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/check_clang_tidy.py:111
 if not any(
-[arg.startswith('-config=') for arg in self.clang_tidy_extra_args]):
+[arg.startswith('-config=') or arg.startswith('-config-file=') for arg 
in self.clang_tidy_extra_args]):
   self.clang_tidy_extra_args.append('-config={}')

ClockMan wrote:
> clang-tidy works with -config and with --config.
> But in documentation there is --config.
> 
> This python script should be updated to accept both single and double dash 
> arguments for config.
Please follow 80 characters limit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144037

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

[PATCH] D143996: [clang-tidy][doc] Remove unused variable

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko requested changes to this revision.
Eugene.Zelenko added inline comments.
This revision now requires changes to proceed.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst:35
 
-Variables: ``a``, ``c``, ``c_ptr1``, ``c_ptr2``, ``c_const_ptr`` and
+Variables: ``a``, ``c``, ``c_ptr1``, ``c_const_ptr`` and
 ``c_reference``, will all generate warnings since they are either:

Please fill as much of 80 symbols as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143996

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

[PATCH] D143851: [clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst:28
 
-   When set to `true` (default is `false`), this check doesn't flag classes 
with a sole, explicitly
-   defaulted destructor. An example for such a class is:
+   When set to `true` (default is `false`), this check will only trigger on 
destructors if they are defined and not defaulted.
 

Please follos 80 characters limit.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst:40
+
+ struct C { // This is not checked, because the destuctor might be 
defaulted in another translation unit.
+   ~C();

ccotter wrote:
> typo: destuctor->destructor
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143851

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

[PATCH] D143843: [clang-tidy][NFC] Remove ModernizeTidyModule::getModuleOptions

[Eugene Zelenko via Phabricator via cfe-commits]

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

But will be good idea to notify authors of checks if they are still active.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143843

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

[PATCH] D142655: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

[Eugene Zelenko via Phabricator via cfe-commits]

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

Looks OK for me, but please wait for other opinion(s).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142655

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

[PATCH] D142655: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:31
+: ClangTidyCheck(Name, Context) {
+  std::optional HeaderFileExtensionsOption =
+  Options.get("HeaderFileExtensions");

Will be good idea to make function in `utils` instead of code duplication. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142655

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

[PATCH] D142592: [clang-tidy][libc] Add an inline function checker for the libc project.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:104
+Checks that all implicit and explicit inline functions in header files are
+tagged with the ``LIBC_INLINE`` macro. See the `libc style guide
+`_ for more information about this 
macro.

One sentence is enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142592

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

[PATCH] D142592: [clang-tidy][libc] Add an inline function checker for the libc project.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please add entry in Release Notes.




Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp:29
+
+  auto SrcBegin = FuncDecl->getSourceRange().getBegin();
+  // Consider functions only in header files.

Please don't use `auto` unless type is explicitly stated in same statement or 
iterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142592

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

[PATCH] D142307: [clang-tidy][NFC] Use C++17 nested namespaces in clang-tidy headers

[Eugene Zelenko via Phabricator via cfe-commits]

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

Looks OK for me, but please fix small formatting issues. Will be good idea to 
await for other eyes.




Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:47
   QualifierPolicy CP = QualifierPolicy::Left);
-} // namespace fixit
-} // namespace utils
-} // namespace tidy
-} // namespace clang
+} // namespace clang::tidy::utils::fixit
 

Please separate with newline.



Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.h:19
 namespace clang {
 class Preprocessor;
+namespace tidy::utils {

Ditto.



Comment at: clang-tools-extra/clang-tidy/utils/IncludeSorter.h:75
 };
-} // namespace tidy
-} // namespace clang
+} // namespace clang::tidy
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_INCLUDESORTER_H

Ditto,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142307

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

[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

D142123  also cries for it :-) Somehow I was 
not able to make comments there :-(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141000

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

[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp:1
+//===--- AvoidCaptureDefaultWhenCapturingThisCheck.cpp - clang-tidy
+//-===//

Ditto.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h:1
+//===--- AvoidCaptureDefaultWhenCapturingThisCheck.h - clang-tidy *- C++
+//-*-===//

Please merge these two lines. Amount of dashes and equal signs could be reduced.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:117
+
+  Warns when lambda specify a capture default and capture ``this``. The check 
also
+  offers fix-its.

`The check also offers fix-its.` could be omitted. This information presents in 
check table.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141133

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

[PATCH] D141769: [clang-tidy][NFC] Use C++17 nested namespaces in add_new_check.py and rename_check.py

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:109-110
 
+- Use C++17 nested namespaces in `add_new_check.py` and `rename_check.py`.
+
 New checks

njames93 wrote:
> Such a trivial change like this probably doesn't need to go in the release 
> notes, @Eugene.Zelenko thoughts?
Probably should not, since entire code base was moved to C++17.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141769

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

[PATCH] D141769: [clang-tidy][NFC] Use C++17 nested namespaces in add_new_check.py and rename_check.py

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/rename_check.py:311
   filename = fileRename(filename, old_module_path, new_module_path)
+  # TODO: remove below replacement when all clang-tidy checks have been
+  # updated with C++17 nested namespaces.

njames93 wrote:
> I'd argue the change to update all checks should go in at the same time.
`Clang-tidy` or just  `all checks`, since script is about Clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141769

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

[PATCH] D141769: [clang-tidy][NFC] Use C++17 nested namespaces in add_new_check.py and rename_check.py

[Eugene Zelenko via Phabricator via cfe-commits]

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

But will be good idea if other eyes will look too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141769

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

[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

[Eugene Zelenko via Phabricator via cfe-commits]

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

Looks OK for me, but please improve Release Notes. Will be good idea to wait 
for developers approval.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:109
 
+- New global configuration file option `HeaderFileExtensions`, replacing the 
check-local
+  option of the same name.

Will be good idea to combine two sentences into one because options are closely 
related.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141000

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

[PATCH] D141583: [clang-tidy][doc] Deprecate the AnalyzeTemporaryDtors option

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:107
+  which is no longer in use. The option will be fully removed in
+  :program:`clang-tidy-18`.
+

But isn't such naming artifact of packaging? May be ":program:`clang-tidy`" 
version 18?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141583

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

[PATCH] D141583: [clang-tidy][doc] Deprecate the AnalyzeTemporaryDtors option

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:106
+- Deprecate the global configuration file option `AnalyzeTemporaryDtors`,
+  which is no longer in use. The option will be fully removed in clang-tidy 18.
+

`Clang-tidy`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141583

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

[PATCH] D141391: [NFC] [clang-tools-extra] Alphabetize clang-tidy release notes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a subscriber: carlosgalvezp.
Eugene.Zelenko added a comment.

In D141391#4040002 , @ccotter wrote:

> I can't land these myself, so if you are able to, would you mind doing so? 
> Thanks!

@carlosgalvezp: Could you please help?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141391

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

[PATCH] D141391: [NFC] [clang-tools-extra] Alphabetize clang-tidy release notes

[Eugene Zelenko via Phabricator via cfe-commits]

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

Thank you for fix! Such issues may be result of rebase artifacts and slip into 
code base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141391

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

[PATCH] D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:219
 
+- Improved :doc:`modernize-loop-convert 
` to
+  refactor container based for loops that initialize iterators with free 
function calls

Please keep alphabetical order (for check names) in this section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140760

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

[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be good idea to check that `HeaderFileExtensions` and 
`ImplementationFileExtensions` do not overlap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141000

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

[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp:32
+  if (Capture.getCaptureKind() == LCK_ByRef) {
+SourceManager  = Context.getSourceManager();
+SourceLocation AddressofLoc = utils::lexer::findPreviousTokenKind(

Should be `const SourceManager &`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141133

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

[PATCH] D141144: [clang-tidy][doc] Improve clang-tidy documentation

[Eugene Zelenko via Phabricator via cfe-commits]

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

But will be good idea to wait for other reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141144

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

[PATCH] D141144: [clang-tidy][doc] Improve clang-tidy documentation

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko accepted this revision.
Eugene.Zelenko added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/docs/clang-tidy/index.rst:142
   e.g. 
--config-file=/some/path/myTidyConfigFile
-This option internally works exactly the 
same way as
+ This option internally works exactly the 
same way as
   --config option after reading specified 
config file.

Is this merge artifact? Same below.



Comment at: clang-tools-extra/docs/clang-tidy/index.rst:215
+--use-color- Use colors in diagnostics. If not set, 
colors
+ will be used if the terminal connected to
+ standard output supports colors.

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141144

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

[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:115
+  Warns when lambda specify a capture default and capture ``this``. The check 
also
+  offers FixIts.
+

`fix-it`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst:6
+
+Default lambda captures in member functions can be misleading about
+whether data members are captured by value or reference. For example,

Please make first statement same as in Release Notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141133

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

[PATCH] D141118: [clang-tidy][NFC] Remove unused User argument in misc-misleading-bidirectional check

[Eugene Zelenko via Phabricator via cfe-commits]

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

Looks OK for me, but please wait for more active developers' opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141118

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:107
 << E->getSourceRange();
+auto LocForEndOfToken =
+Lexer::getLocForEndOfToken(E->getEndLoc(), 0, SM, LangOpts);

Please don't use `auto` unless type is explicitly stated in same statement or 
iterator.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:221
 << IndexExpr->getSourceRange();
+auto LocForEndOfToken =
+Lexer::getLocForEndOfToken(IndexExpr->getEndLoc(), 0, SM, LangOpts);

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D140968: [clang-tidy] Add check for passing the result of `std::string::c_str` to `strlen`

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/StrlenStringCStrCheck.h:1
+//===--- StrlenStringCStrCheck.h - clang-tidy *- C++ -*-===//
+//

Should be 80 characters long. See other checks as example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140968

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