[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:58
+  
+  Warns Abseil users if they attempt to depend on internal details.
 

I think will be good idea to omit user, and just refer to code which depend on 
internal details. Please synchronize with documentations.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:15
+}
+class SomeContainer{};
+

Please separate with empty line.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:51
+
+
+

Please remove two empty lines


https://reviews.llvm.org/D50542



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Check documentation is missing.




Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:21
+void NoNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus) return;
+

Please place return in separate line.



Comment at: clang-tidy/abseil/NoNamespaceCheck.h:25
+class NoNamespaceCheck : public ClangTidyCheck {
+ public:
+  NoNamespaceCheck(StringRef Name, ClangTidyContext *Context)

Please run Clang-format.



Comment at: docs/ReleaseNotes.rst:60
 
 The improvements are...
 

Please remove placeholder.



Comment at: docs/ReleaseNotes.rst:65
+
+  Checks to ensure user did not open namespace absl as that
+  violates abseil's compatibility guidelines.

Please reformulate to refer to code, not user. Please enclose absl in ``.



Comment at: docs/ReleaseNotes.rst:66
+  Checks to ensure user did not open namespace absl as that
+  violates abseil's compatibility guidelines.
+

abseil ->Abseil.


https://reviews.llvm.org/D50580



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

[PATCH] D48909: [clang-doc] Update BitcodeReader to use llvm::Error

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

You could use //Differential revision: // in commit description to 
close review automatically.


https://reviews.llvm.org/D48909



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

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst:6
+
+This check triggers on calls to ``absl::StrSplit()`` or ``absl::MaxSplits()``
+where the delimiter is a single character string literal. The check will offer

Please make first statement same as in Release Notes. Please also avoid //this 
check//.



Comment at: docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst:21
+  for (auto piece : absl::StrSplit(str, "B")) {
+  // Suggested - the argument is a character, which causes the more efficient
+  // overload of absl::StrSplit() to be used.

Please separate before and after with empty line. Same in other places.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50862



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

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Somehow documentation file was not committed.


https://reviews.llvm.org/D50389



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please rebase from trunk.




Comment at: docs/ReleaseNotes.rst:63
+
+  Checks to ensure code does not open `namespace absl` as that
+  violates Abseil's compatibility guidelines.

Just Ensures. Please use `` for language constructs.



Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:6
+
+This check gives users a warning if they attempt to open ``namespace absl``.
+Users should not open ``namespace absl`` as that conflicts with abseil's

Please synchronize first sentence with Release Notes.



Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:7
+This check gives users a warning if they attempt to open ``namespace absl``.
+Users should not open ``namespace absl`` as that conflicts with abseil's
+compatibility guidelines and may result in breakage.

Abseil



Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:10
+
+Any user that uses:
+

Code instead of user, please.



Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:20
+
+See `the full Abseil compatibility guidelines `_ for more information.

Please remove space after https:


https://reviews.llvm.org/D50580



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`abseil-no-namespace
+  ` check.

Please sort new checks list alphabetically.



Comment at: docs/ReleaseNotes.rst:79
 
+>>> .r340314
 Improvements to include-fixer

Please remove merge conflict residual.


https://reviews.llvm.org/D50580



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

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:70
+
+  Flags uses of ``absl::StrCat()`` to append to a std::string. Suggests 
+  ``absl::StrAppend()`` should be used instead.

Please enclose std::string into ``



Comment at: docs/ReleaseNotes.rst:72
+  ``absl::StrAppend()`` should be used instead.
+  Does not diagnose cases where ``abls::StrCat()`` is used as a template 
+  argument for a functor.

Details belongs to documentation.


https://reviews.llvm.org/D51061



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:116
+
+  Checks for allowed system includes and suggests removal of any others. If no
+  includes are specified, the check will exit without issuing any warnings.

Is it necessary to highlight that warnings will not be emitted in case of 
disallowed headers are not found? Same in documentation.


https://reviews.llvm.org/D43778



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

[PATCH] D46602: [clang-tidy] Store checks profiling info as CSV files

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I think will be good idea to store data in JSON format too.




Comment at: docs/ReleaseNotes.rst:60
 
+- clang-tidy learned to store checks profiling info as CSV files.
+

May be //Profile information could be stored in SSV format.// sounds better?



Comment at: docs/clang-tidy/index.rst:762
+
+To enable profiling info collection, use ``-enable-check-profile`` argument.
+The timings will be outputted to the ``stderr`` as a table. Example output:

Please use ` for command line options and other non-C/C++ constructs.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

See also PR22165.


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

>>> The warning is off by default.
>> 
>> We typically do not add off-by-default warnings because experience has shown 
>> the rarely get enabled in practice. Can you explain a bit more about why 
>> this one is off by default?
> 
> Right. I believe this is going to be used in practice, the reason I'm adding 
> it involves some user demand for such warning. Such quoted include use in 
> frameworks happen often and we would like a smooth transition to happen here 
> (e.g. do not initially affect -Werror users). If it proves worth it, we can 
> migrate to on by default in the future. It wouldn't be a problem if we have 
> it on by default on open source and disable by default downstream, but I 
> rather be consistent.

You could just not include new warning into -Wall and -Wextra. Those who will 
want to check #include statements correctness could enable it explicitly.


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D47157#1110430, @bruno wrote:

> Hi Eugene,
>
> > You could just not include new warning into -Wall and -Wextra. Those who 
> > will want to check #include statements correctness could enable it 
> > explicitly.
>
> This is exactly what's happening in the patch, the new warning isn't part of 
> `-Wall` or `-Wextra`, and is marked `DefaultIgnore`, which means that will 
> fire only when `-Wquoted-include-in-framework-header` is passed to the 
> driver. Am I missing something from your explanation?
>
> Thanks,


Thank you for clarification! Sorry, I didn't know TableGen syntax well enough 
to deduce this from source.


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Same mistake  could be made with new[] operator.




Comment at: docs/clang-tidy/checks/list.rst:10
android-cloexec-creat
+   android-cloexec-dup
android-cloexec-epoll-create

I think will be better to fix order of other checks separately from this check.



Comment at: 
docs/clang-tidy/checks/misc-misplaced-operator-in-strlen-in-alloc.rst:7
+Finds cases a value is added to or subtracted from the string in the parameter
+of ``strlen()`` method instead of to the result and use its return value as an
+argument of a memory allocation function (``malloc()``, ``calloc()``,

strlen() is function, not method.


https://reviews.llvm.org/D39121



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

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst:16
+
+   string s = StrCat("A", StrCat("B", StrCat("C", "D")));
+   ==> string s = StrCat("A", "B", "C", "D");

Please add namespaces and use empty line instead of ==>


https://reviews.llvm.org/D51132



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

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst:16
+
+   string s = absl::StrCat("A", absl::StrCat("B", absll::StrCat("C", 
"D")));
+   string s = absl::StrCat("A", "B", "C", "D");

std::string. Please insert empty line.



Comment at: docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst:17
+   string s = absl::StrCat("A", absl::StrCat("B", absll::StrCat("C", 
"D")));
+   string s = absl::StrCat("A", "B", "C", "D");
+   absl::StrAppend(&s, absl::StrCat("E", "F", "G"));

std::string. Same indentation as in previous line. Same for second example.


https://reviews.llvm.org/D51132



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

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-str-cat-append.rst:11
+
+  a = StrCat(a, b); // Use StrAppend(&a, b) instead.
+

Please add namespace.


https://reviews.llvm.org/D51061



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

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Example in documentation was not fixed.




Comment at: docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst:16
+
+   std::string s = absl::StrCat("A", absl::StrCat("B", absll::StrCat("C", 
"D")));
+ std::string s = absl::StrCat("A", "B", "C", "D");

Will be good idea to add before/after comments. Indentation is not fixed and 
empty line was not inserted. Same or second example.


https://reviews.llvm.org/D51132



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

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Indentation of example in documentation is still fixed.


https://reviews.llvm.org/D51132



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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I still thik will be good idea to rename check (deps -> dependencies).


https://reviews.llvm.org/D50542



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

[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`google-objc-function-naming
+  ` check.

Please use alphabetical order.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51575



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

[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:29
+
+namespace {
+std::string GetIsolatedDecl(const VarDecl *D, const SourceManager &SM,

Please use static instead.



Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:37
+  VarType.getAsString(TypePrinter) + " " + D->getNameAsString();
+  std::string Initializer = "";
+

See readability-redundant-string-init.



Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:60
+  const auto *WholeDecl = Result.Nodes.getNodeAs("decl_stmt");
+  std::string Replacement = "";
+

See readability-redundant-string-init.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949



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

[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I would suggest to use declaration or declarations in check name instead of 
abbreviation.

Actually there are existing coding guidelines for thus rule, like SEI CERT 
.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949



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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.h:42
+return false;
+  };
+  bool isSyntheticValue(const clang::SourceManager *SourceManager,

Unnecessary semicolon. Please also add empty line after.



Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:55
+
+By default only 0, 1 and -1 integer values are accepted without a warning.
+This can be overriden with the 'IgnoredIntegerValues' option.  In addition,

Please enclose numbers in `. Same for 0.0.



Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:56
+By default only 0, 1 and -1 integer values are accepted without a warning.
+This can be overriden with the 'IgnoredIntegerValues' option.  In addition,
+the 0.0 floating point value is accepted. There is no configuration for

Please use `.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:56
+By default only `0`, `1` and `-1` integer values are accepted without a 
warning.
+This can be overridden with the 'IgnoredIntegerValues' option.  In addition,
+the `0.0` floating point value is accepted. There is no configuration for

Sorry, almost forgot. Please prefix IgnoredIntegerValues with :option:. You 
still need to use ` for it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49890: Clang-Tidy Export Problem

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: ClangTidy.cpp:627
+Files->makeAbsolutePath(ErrorAbsoluteFilePath);
+if (SingleErrors.find(ErrorAbsoluteFilePath) == SingleErrors.end())
+{

Please run Clang-format.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49890



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

[PATCH] D49890: Clang-Tidy Export Problem

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: ClangTidy.cpp:620
+  llvm::StringMap SingleErrors;
+  for (const ClangTidyError &Error : Errors) {
+if (!Error.BuildDirectory.empty()) {

You could use auto, since it's iterator over container.



Comment at: ClangTidy.cpp:621
+  for (const ClangTidyError &Error : Errors) {
+if (!Error.BuildDirectory.empty()) {
+  FileSystem.setCurrentWorkingDirectory(Error.BuildDirectory);

Curly braces are not necessary.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49890



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

[PATCH] D49890: Clang-Tidy Export Problem

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: ClangTidy.cpp:612
+
+  FileManager *Files = new FileManager(FileSystemOptions());
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();

You could use auto here, because type is in new statement.



Comment at: ClangTidy.cpp:614
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();
+  auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
+  if (!InitialWorkingDir)

Type is not obvious, so please don't use auto.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49890



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

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).




Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:24
+
+StringRef CheckPPC(StringRef Name) {
+  if (Name.startswith("vec_"))

Please make function static instead of using anonymous namespace. See LLVM 
Coding Style.



Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:28
+  else
+return "";
+

I think returning {} will be better. Same for trailing return.



Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:47
+
+StringRef CheckX86(StringRef Name) {
+  if (Name.startswith("_mm_"))

Please make function static instead of using anonymous namespace. See LLVM 
Coding Style.



Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:55
+  else
+return "";
+

I think returning {} will be better. Same for trailing return.



Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:77
+void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  
callExpr(callee(functionDecl(matchesName("^::(_mm_|_mm256_|_mm512_|vec_)"))),

You should enable this check only when compiling in appropriate C++ version. 
See getLangOpts() usage in other checks.



Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.h:30
+
+ private:
+};

Please remove private section and empty line before it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42983



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

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:77
+void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  
callExpr(callee(functionDecl(matchesName("^::(_mm_|_mm256_|_mm512_|vec_)"))),

MaskRay wrote:
> Eugene.Zelenko wrote:
> > You should enable this check only when compiling in appropriate C++ 
> > version. See getLangOpts() usage in other checks.
> Thx, I didn't know getLangOpts() before.
> 
> I think even in `-x c` mode, `::` is still the prefix of an identifier.
> 
>   matchesName("^(_mm_|_mm256_|_mm512_|vec_)")
> 
> doesn't match anything but 
> 
>   matchesName("^::(_mm_|_mm256_|_mm512_|vec_)")
> 
> matches.
> 
> verified via `clang-tidy -checks='-*,readability-simd-intrinsics' a.c -- -xc`
> 
Check should be enabled only for C++ version which has std::experimental::simd 
implementation, so this is why you need to use getLangOpts().


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42983



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

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:97
 
+- New `readability-simd-intrinsics
+  
`_
 check

Please move it new checks section.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42983



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

[PATCH] D43089: clang: Add ARCTargetInfo

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:8220
+
+} // End anonymous namespace.
 

Please change to // namespace


https://reviews.llvm.org/D43089



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

[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/misc-throw-keyword-missing.rst:6
+
+This check warns about the potentially missing `throw` keyword. If a temporary 
object is created,
+but the object's type derives from (or the same as) a class that has 
'EXCEPTION', 'Exception' or

aaron.ballman wrote:
> about the potentially -> about a potentially
Please remove //This check// and enclose throw in ``


https://reviews.llvm.org/D43120



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

[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/misc/ThrowKeywordMissingCheck.cpp:24
+
+  auto CtorInitializerList =
+  cxxConstructorDecl(hasAnyConstructorInitializer(anything()));

Please don't use auto where type could not be easily deduced.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43120



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

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+- New `fuchsia-add-visibility
+  
`_ 
check

Please move into new checks section.



Comment at: docs/ReleaseNotes.rst:63
+
+  Check to find a given named function and suggest a fix to add a
+  `__attribute__((visibility("hidden")))` to its definition.

Check to find -> Finds.



Comment at: docs/clang-tidy/checks/fuchsia-add-visibility.rst:6
+
+Check to find a given named function and suggest a fix to add
+a `__attribute__((visibility("hidden")))` to its definition.

Check to find -> Finds.


https://reviews.llvm.org/D43392



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

[PATCH] D43500: modernize-use-default-member-init: Remove trailing comma and colon.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:15
 
+#include "../utils/LexerUtils.h"
+

Please place it on a top, since header belong to project.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:26
+   tok::TokenKind K) {
+  for (;;) {
+Token Tok = utils::lexer::getPreviousToken(*Context, Location);

Please use while (true) instead.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:48
+  auto NextInit = std::next(pos);
+  if (NextInit != Ctor->init_end()) {
+return (*NextInit)->getSourceLocation();

Unnecessary curvy brackets.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:177
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true) != 0),
+  RemovedInitializers() {}
 

Unnecessary initializer. See [[ 
http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-member-init.html
 | readability-redundant-member-init ]].



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:271
+
+  if (++RemovedInitializers[Ctor] == Ctor->getNumCtorInitializers()) {
+removeTrailingColon(Diag, Result.Context, Ctor);

Unnecessary curvy brackets.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:298
+if (sameValue(NextInit->getMember()->getInClassInitializer(),
+  NextInit->getInit())) {
+  // The next initializer will be removed later. Removing only the

Unnecessary curvy brackets.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:302
+  Diag << FixItHint::CreateRemoval(Init->getSourceRange());
+} else {
+  // The next initializer will not be remove. In this case, we should 
create

Unnecessary curvy brackets.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:308
+}
+  } else {
+// This initializer is in the middle of other one.

Unnecessary curvy brackets.



Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:317
+
+  if (++RemovedInitializers[Ctor] == Ctor->getNumCtorInitializers()) {
+removeTrailingColon(Diag, Result.Context, Ctor);

Unnecessary curvy brackets.



Comment at: unittests/clang-tidy/ModernizerModuleTest.cpp:62
+}
+} // namespace test
+} // namespace tidy

Please separate function from namespaces ends with empty line.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43500



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

[PATCH] D43667: [clang-doc] Implement a YAML generator

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please run Clang-format and Clang-tidy modernize over new code.




Comment at: clang-doc/generators/Generators.h:29
+  Generator(std::unique_ptr &IS, StringRef Root, StringRef Format)
+  : IS(IS), Root(Root), Format(Format){};
+  virtual ~Generator(){};

Unnecessary ; after constructor body. Please enable Clang's -Wextra-semi



Comment at: clang-doc/generators/Generators.h:30
+  : IS(IS), Root(Root), Format(Format){};
+  virtual ~Generator(){};
+

Please use = default;



Comment at: clang-doc/generators/Generators.h:47
+  YAMLGenerator(std::unique_ptr &IS, StringRef Root, StringRef Format)
+  : Generator(IS, Root, Format){};
+  virtual ~YAMLGenerator(){};

Unnecessary ; after constructor body.



Comment at: clang-doc/generators/Generators.h:48
+  : Generator(IS, Root, Format){};
+  virtual ~YAMLGenerator(){};
+

Please use = default;



Comment at: test/clang-doc/namespace-yaml.cpp:11
+void f();
+void f() {};
+

Unnecessary ; after function body.


https://reviews.llvm.org/D43667



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

[PATCH] D43667: [clang-doc] Implement a YAML generator

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I may be mistaken, but Clang source code didn't use llvm namespace by default. 
Insetad you should include clang/Basic/LLVM.h and use llvm:: for rest of things.




Comment at: clang-doc/tool/ClangDocMain.cpp:42
 
 namespace {
 

There is no need to enclose static definitions in anonymous namespace.


https://reviews.llvm.org/D43667



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

[PATCH] D41102: Setup clang-doc frontend framework

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please run Clang-format and Clang-tidy modernize.




Comment at: clang-doc/Representation.h:80
+  : LineNumber(LineNumber), Filename(std::move(Filename)) {}
+  int LineNumber;
+  std::string Filename;

Please separate constructors from data members with empty line.


https://reviews.llvm.org/D41102



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please run Clang-format and Clang-tidy modernize.




Comment at: clang-doc/BitcodeReader.h:36
+ public:
+  ClangDocBitcodeReader() {}
+  using RecordData = SmallVector;

Please use = default;


https://reviews.llvm.org/D43341



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

[PATCH] D43424: [clang-doc] Implement a (simple) Markdown generator

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please run Clang-format and Clang-tidy modernize.




Comment at: clang-doc/generators/Generators.h:46
+public:
+  MDGenerator(std::unique_ptr &IS, StringRef Root, StringRef Format) 
: Generator(IS, Root, Format) {};
+  virtual ~MDGenerator() {};

Please remove semicolon after constructor body. Please enable CLang's 
-Wextra-semi.



Comment at: clang-doc/generators/Generators.h:47
+  MDGenerator(std::unique_ptr &IS, StringRef Root, StringRef Format) 
: Generator(IS, Root, Format) {};
+  virtual ~MDGenerator() {};
+  

Please use = default;


https://reviews.llvm.org/D43424



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/fuchsia/CMakeLists.txt:8
   OverloadedOperatorCheck.cpp
+  RestrictIncludesCheck.cpp
   StaticallyConstructedObjectsCheck.cpp

Will be good idea to be have consistent name and documentation terminology: 
include versus header.



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:48
+
+  typedef std::vector FileIncludes;
+  std::map IncludeDirectives;

Please use using and run Clang-tidy modernize.



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:70
+  for (auto &Bucket : IncludeDirectives) {
+auto &FileDirectives = Bucket.second;
+

Please don't use auto here, since type is not obvious.



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:80
+  unsigned ToLen =
+  std::strcspn(SM.getCharacterData(Include.Range.getBegin()), "\n") + 
1;
+  auto ToRange = CharSourceRange::getCharRange(

Please include .



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:81
+  std::strcspn(SM.getCharacterData(Include.Range.getBegin()), "\n") + 
1;
+  auto ToRange = CharSourceRange::getCharRange(
+  Include.Loc, ToLoc.getLocWithOffset(ToLen));

Please don't use auto here, since type is not obvious.



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:101
+  Compiler.getPreprocessor().addPPCallbacks(
+  ::llvm::make_unique(
+  *this, Compiler.getSourceManager()));

I don't think that you should prefix llvm with ::



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.h:25
+class RestrictIncludesCheck : public ClangTidyCheck {
+ public:
+  RestrictIncludesCheck(StringRef Name, ClangTidyContext *Context)

Please run Clang-format over code.



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.h:30
+Options.get("Includes", "::std::vector"))) {}
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;

Please separate constructor with empty line from rest of methods.


https://reviews.llvm.org/D43778



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

[PATCH] D33470: [clang-tidy] Add misc-default-numerics

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/misc-default-numerics.rst:11
+Consider scenario:
+1. Have `typedef long long BigInt` in source code
+2. Use `std::numeric_limits::min()`

May be code-block will be better?


https://reviews.llvm.org/D33470



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

[PATCH] D33497: clang-tidy check for __func__/__FUNCTION__ in lambdas

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).


https://reviews.llvm.org/D33497



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

[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It's also necessary to mention new checks group in docs/clang-tidy/index.rst.


Repository:
  rL LLVM

https://reviews.llvm.org/D33304



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

[PATCH] D33497: [clang-tidy] check for __func__/__FUNCTION__ in lambdas

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:83
+
+   Finds uses of __func__ or __FUNCTION__ inside lambdas.
+

Please highlight __func__ and __FUNCTION__ with ``.


https://reviews.llvm.org/D33497



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

[PATCH] D33497: [clang-tidy] check for __func__/__FUNCTION__ in lambdas

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:83
+
+   Finds uses of __func__ or __FUNCTION__ inside lambdas.
+

Eugene.Zelenko wrote:
> Please highlight __func__ and __FUNCTION__ with ``.
I meant double ``, not single `.


https://reviews.llvm.org/D33497



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

[PATCH] D33537: [clang-tidy] Exception Escape Checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).




Comment at: docs/clang-tidy/checks/misc-exception-escape.rst:7
+Finds functions which should not throw exceptions:
++ Destructors
++ Copy constructors

I think - or * is more correct.



Comment at: docs/clang-tidy/checks/misc-exception-escape.rst:10
++ Copy assignment operators
++ The main() functions
++ swap() functions

Please enclose main() in ``. Same for other functions and keywords.



Comment at: docs/clang-tidy/checks/misc-exception-escape.rst:29
+   Comma separated list containing function names which should not throw. An
+   example for using this parameter is the function WinMain in the Windows API.
+   Default vale is empty string.

Please add () to WinMain and enclose it in ``.



Comment at: docs/clang-tidy/checks/misc-exception-escape.rst:35
+   Comma separated list containing type names which are not counted as thrown
+   exceptions in the checker. Default value is empty string.

Please use check instead of checker.


https://reviews.llvm.org/D33537



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

[PATCH] D33304: [clang-tidy] Add a new module Android and a new check for file descriptors.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/utils/ExprToStr.cpp:23
+ SM.getCharacterData(TE) - SM.getCharacterData(B));
+}
+} // namespace utils

Please add empty line.



Comment at: clang-tidy/utils/ExprToStr.h:22
+std::string ExprToStr(const Expr *EX, const SourceManager &SM,
+  const LangOptions &LangOpts);
+} // namespace utils

Please add empty line.



Comment at: docs/ReleaseNotes.rst:69
+
+  Checks if any usage of function ``creat()``.
+

May be detect usage of ``creat()`` will be better?



Comment at: docs/clang-tidy/checks/android-creat-usage.rst:5
+===
+The usage of creat() is not recommended, it's better to use open().

Please enclose creat() and open() in `` and add empty line before.



Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:5
+==
+A common source of security bugs has been code that opens file without using
+the ``O_CLOEXEC`` flag.  Without that flag, an opened sensitive file would

Please add empty line.



Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:10
+``open64()`` must include ``O_CLOEXEC`` in their flags argument.
+
+

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/android-fopen-mode.rst:5
+=
+``fopen()`` should include ``e`` in their mode string; so ``re`` would be
+valid.

Please add empty line.


https://reviews.llvm.org/D33304



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

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).




Comment at: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst:29
+
+The checker suggests a FixItHint in every scenario including multiple 
+missing initializers and constructors with template argument.

checker -> check, FixItHint -> fix-it.



Comment at: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst:31
+missing initializers and constructors with template argument.
\ No newline at end of file


Please add newline.


https://reviews.llvm.org/D33722



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

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:83
+
+  Finds copy constructors where the ctor don't call the constructor of the 
base class.
+

I think will be good idea to replace ctor with constructor. Or re-phrase 
sentence to avoid repetition of word constructor.


https://reviews.llvm.org/D33722



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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/cert/CERTTidyModule.cpp:64
+"cert-msc54-cpp");
 // C checkers
 // DCL

checker -> check.



Comment at: docs/ReleaseNotes.rst:73
+
+  Checks if a signal handler is not a plain old function.
+

Probably C++ should be mentioned for clarity.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:7
+This check will give a warning if a signal handler is not defined
+as an 'extern C' function or if the declaration of the function
+contains C++ representation.

'extern C' -> ``extern "C"``.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:21
+}
+
+

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:32
+}
+
+

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:34
+
+This check corresponds to the CERT CPP Coding Standard rule
+`MSC54-CPP. A signal handler must be a plain old function

Please replace CPP with C++ as in similar checks documentation.


Repository:
  rL LLVM

https://reviews.llvm.org/D33825



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

[PATCH] D33841: [clang-tidy] redundant keyword check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).




Comment at: docs/clang-tidy/checks/readability-redundant-keyword.rst:6
+
+This checker removes the redundant `extern` and `inline` keywords from code.
+

checker -> check. Please use `` to highlight language constructs here and below.



Comment at: docs/clang-tidy/checks/readability-redundant-keyword.rst:13
+  extern void h();
+
+

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/readability-redundant-keyword.rst:20
+  class X {
+inline int f() {  }
+  };

May be three dots will be better as common punctuation?



Comment at: docs/clang-tidy/checks/readability-redundant-keyword.rst:22
+  };
\ No newline at end of file


Please add newline.


Repository:
  rL LLVM

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant keyword check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D33841#771944, @Prazek wrote:

> Feature request: removing "void" from int main(void)


This will duplicate modernize-redundant-void-arg.


Repository:
  rL LLVM

https://reviews.llvm.org/D33841



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

[PATCH] D34206: [clang-tidy] Add "MakeSmartPtrFunction" option to modernize-make-shared/unique checks.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be good idea to run modernize-make-unique on LLVM/Clang/etc for 
llvm::make_unique.


https://reviews.llvm.org/D34206



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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:169
+  }
+private:
+  SmallVector CheckNames;

Please separate with empty line.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:170
+private:
+  SmallVector CheckNames;
+  size_t NolintIndex = 0;

Please include header for SmallVector.h.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:339
 std::unique_ptr OptionsProvider)
 : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
+  Profile(nullptr),

Please use default members initialization for DiagEngine and Profile.


https://reviews.llvm.org/D41326



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

[PATCH] D41363: [clang-tidy] Adding Fuchsia checker for overloaded operators

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:141
+
+  Warns if an operator is overloaded, except for the copy and move operators.
+

assignment operators?



Comment at: docs/clang-tidy/checks/fuchsia-overloaded-operator.rst:6
+
+Warns if an operator is overloaded, except for the copy and move operators.
+

assignment operators?


https://reviews.llvm.org/D41363



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

[PATCH] D41546: [clang-tidy] Adding Fuchsia checker for statically constructed objects

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:146
+
+  Warns if statically-stored objects are created, unless constructed with 
`constexpr`.
+

Please highlight constexpr with ``, not `.



Comment at: docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst:6
+
+Warns if statically-stored objects are created, unless constructed with 
+`constexpr`.

Please highlight constexpr with ``, not `.



Comment at: docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst:25
+
+But creation of static objects using `constexpr` constructors is allowed:
+

Please highlight constexpr with ``, not `.


https://reviews.llvm.org/D41546



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:13
+#include "clang/Lex/PPCallbacks.h"
+
+namespace clang {

Please include string and STLExtras.h.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:8
+constructs exist for the task.
+

Will be good idea to add link to relevant section in documentation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:13
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;

Please include cassert, Regex.h, raw_ostream.h, SmallString.h.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.h:14
+#include "../ClangTidy.h"
+
+namespace clang {

Please include string.



Comment at: docs/clang-tidy/checks/bugprone-unused-return-value.rst:17
+
+   - ``std::async``. Not using the return value makes the call synchronous.
+   - ``std::launder``. Not using the return value usually means that the

Please add round brackets to all functions/methods here and below.


https://reviews.llvm.org/D41655



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

[PATCH] D41740: [clang-tidy] Adding a new bugprone check for streaming objects of type int8_t or uint8_t

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:1
+//===--- StreamInt8Check.cpp - 
clang-tidy--===//
+//

Please add real description, use space after it and shorten to 80 symbols.



Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:25
+hasArgument(0, expr(hasType(qualType(hasCanonicalType(
+
hasDeclaration(cxxRecordDecl(hasName("std::basic_ostream",
+hasArgument(1, expr(hasType(hasCanonicalType(

May be match should be configurable, so other streams could be analyzed too? 
How about LLVM's raw_ostream?



Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:36
+const auto *MatchedExpr = 
Result.Nodes.getNodeAs("oper");
+const auto *Offender = MatchedExpr->getArg(1);
+

Please don't use auto in cases where return type could not be easily deduced. 
See 
[[http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html|modernize-use-auto]].
 Same for //auto Underlying//.



Comment at: clang-tidy/bugprone/StreamInt8Check.h:1
+//===--- StreamInt8Check.h - clang-tidy---*- C++ -*-===//
+//

Please add real description and use space after it.



Comment at: clang-tidy/bugprone/StreamInt8Check.h:19
+
+/// FIXME: Write a short description.
+///

Please write real description here.



Comment at: docs/ReleaseNotes.rst:62
+  
`_ 
check
+
 - New module `fuchsia` for Fuchsia style checks.

Please move to new checks section in alphabetical order and add one statement 
description (usually first statement from documentation).



Comment at: test/clang-tidy/bugprone-stream-int8.cpp:32
+
+void awesome_f2(std::ostream& os, uint8_t i) {
+using UC = unsigned char;

Please use meaningful name or main.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41740



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

[PATCH] D41815: [clang-tidy] implement check for goto

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:63
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.

I think will be good idea to reformulate this statement and its copy in 
documentation. //diagnosed with this check// is tautological for any check.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



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

[PATCH] D43847: [tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please add new module in docs/clang-tidy/index.rst and mention it in release 
notes.




Comment at: clang-tidy/absl/AbslTidyModule.cpp:14
+#include "StringFindStartswithCheck.h"
+using namespace clang::ast_matchers;
+

Please separate with empty line.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:40
+void StringFindStartswithCheck::check(const MatchFinder::MatchResult &result) {
+  const auto &context = *result.Context;
+  const auto &source = context.getSourceManager();

Please don't use auto when type could not be deduced from same statement. Same 
in other places.



Comment at: clang-tidy/absl/StringFindStartswithCheck.h:30
+private:
+  std::unique_ptr include_inserter_;
+};

Please include 



Comment at: docs/ReleaseNotes.rst:63
+
+  Checks whether a string::find result is compared with 0, and suggests
+  replacing with absl::StartsWith.

string::find -> std::string::find(), absl::StartsWith -> absl::StartsWith(). 
Please enclose them in ``.



Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:6
+
+This check triggers on (in)equality comparisons between string.find()
+and zero. Comparisons like this should be replaced with

Please make first statement same as in release notes and avoid //This check//.



Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:11
+
+::
+

Please use .. code-block: c++



Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:17
+should be replaced with
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+

Please use .. code-block: c++



Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:18
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+

Is there any online documentation about such usage? If so please refer to in 
at. See other guidelines as example.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

std::basic_string::starts_with() was suggested for C++20. May be will be good 
idea to generalize code to create absl and modernize checks?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43870: [clang-tidy] Another batch of checks to rename from misc- to bugprone-.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+- The 'misc-undelegated-constructor' check was renamed to 
`bugprone-undelegated-constructor
+  
`_

Please sort checks alphabetically. Will be also good idea to move renamed 
checks after new checks/modules.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43870



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D43847#1023452, @hokein wrote:

> In https://reviews.llvm.org/D43847#1021967, @aaron.ballman wrote:
>
> > I need a bit more context because I'm unfamiliar with `absl`. What is this 
> > module's intended use?
>
>
> As `absl` has been open-sourced (https://github.com/abseil/abseil-cpp), I 
> think there will be more absl-related checks contributed from google or 
> external contributors in the future, so it make sense to create a new module.
>
> @niko, could you please refine the code style of this patch to follow the 
> LLVM code standard 
> 
>  (especially the variable name, should be camel case "VariableName")?


May be //abseil// is better name for module?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

By the word, may be similar check for std::string::rfind() and 
std::string::ends_with() (does abseil have analog) should be added too?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D43847#1025833, @niko wrote:

> Thanks everyone for your comments! I renamed the namespace and filenames to 
> 'abseil'.
>
> @Eugene.Zelenko, definitely interested in extending this to a C++20 modernize 
> check and adding `absl::EndsWith()` support,  would it be OK though to do 
> this in a later patch?


I think will be better to do this now, since you'll need to create base class 
and specializations for Abseil and C++17.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D41102: Setup clang-doc frontend framework

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-doc/BitcodeWriter.h:160
+class ClangDocBitcodeWriter {
+ public:
+  ClangDocBitcodeWriter(llvm::BitstreamWriter &Stream,

Looks like Clang-format was applied incorrectly, because this is Google, not 
LLVM style. Please note that it doesn't modify file, just output formatted code 
to terminal.

Please reformat other files, including those in dependent patches.


https://reviews.llvm.org/D41102



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

[PATCH] D44173: [clang-tidy] Add "portability" module and rename readability-simd-intrinsics to portability-simd-intrinsics

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new module in Release Notes. I think new modules should be 
before new checks there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44173



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

[PATCH] D44173: [clang-tidy] Add "portability" module and rename readability-simd-intrinsics to portability-simd-intrinsics

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+- New module `portability` and new `portability-simd-intrinsics
+  
`_
 check

Module and check from it should be mentioned separately. See Release Notes for 
previous release.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44173



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

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:26
+
+bool IsParentOf(const CXXRecordDecl *Parent, const CXXRecordDecl *ThisClass) {
+  assert(Parent);

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:40
+
+bool IsRecursiveParentOf(const CXXRecordDecl *Parent,
+ const CXXRecordDecl *ThisClass) {

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:48
+std::list
+GetParentsByGrandParent(const CXXRecordDecl *GrandParent,
+const CXXRecordDecl *ThisClass) {

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:66
+// PrintingPolicy for anonymous namespaces.
+std::string GetNameAsString(const NamedDecl &Decl) {
+  PrintingPolicy PP(Decl.getASTContext().getPrintingPolicy());

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:91
+void ParentVirtualCallCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("call");

Please remove this line.



Comment at: docs/ReleaseNotes.rst:76
+
+  Warns if one calls grand-..parent's virtual method in child's virtual
+  method instead of parent's. Can automatically fix such cases by retargeting

Please make description here and first statement in documentation same.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:9
+
+class A {
+...

Please add .. code-block:: c++


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



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

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:12
+
+#include 
+#include 

Please run Clang-format and remove empty line between headers.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



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

[PATCH] D44346: [clang-tidy] Add Fuchsia checker for temporary objects

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/fuchsia/ZxTemporaryObjectsCheck.cpp:24
+  std::string QualifiedName = Node.getQualifiedNameAsString();
+  return llvm::any_of(Names,
+  [&](StringRef Name) { return QualifiedName == Name; });

Please include STLExtras.h and string.



Comment at: clang-tidy/fuchsia/ZxTemporaryObjectsCheck.h:31
+Names(utils::options::parseStringList(
+Options.get("Names", "::std::vector"))) {}
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;

This contradict documentation which claims that default is empty.



Comment at: docs/ReleaseNotes.rst:79
+  Warns on construction of specific temporary objects in the Zircon kernel.
+  Takes the list of such discouraged temporary objects as a parameter.
+

I think you could omit second statement.



Comment at: docs/clang-tidy/checks/fuchsia-zx-temporary-objects.rst:38
+
+   A semi-colon-separated list of fully-qualified names of C++ classes that 
should not be constructed as temporaries. Default is empty.

Please use 2 spaces indentation and 80 characters limit. Continuation should be 
indented too.


https://reviews.llvm.org/D44346



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

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:47
+const CXXRecordDecl *ThisClass) {
+
+  assert(GrandParent != nullptr);

Please remove empty line.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:116
+
+  if (IsParentOf(CastToType, ThisType)) {
+return;

No need for brackets.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



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

[PATCH] D44143: [clang-tidy] Create properly seeded random generator check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:63
+
+  Detects inappropriate seeding of C++ random generators and C srand function.
+

Please replace srand  with ``srand ()``.



Comment at: docs/clang-tidy/checks/cert-msc51-cpp.rst:7
+This check flags all pseudo-random number engines, engine adaptor
+instantiations and srand when initialized or seeded with default argument,
+constant expression or any user-configurable type. Pseudo-random number

aaron.ballman wrote:
> Backticks around `srand`
Two of them and please add (). Will be good idea to make first statement same 
as in Release Notes.



Comment at: docs/clang-tidy/checks/cert-msc51-cpp.rst:29
+  }
+
+Options

Is there guideline documentation available online? If so, please add link. See 
other checks documentation as example.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143



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

[PATCH] D44346: [clang-tidy] Add Zircon module to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:77
 
+- New `zircon-temporary-objects
+  
`_ 
check

Please sort new checks in alphabetical order.


https://reviews.llvm.org/D44346



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

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:122
 
+- Added `VariableThreshold` to `readability-function-size
+  
`_
 check

Will be good idea to add //option// to clarify improvement.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44602



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

[PATCH] D43424: [clang-doc] Implement a (simple) Markdown generator

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-doc/MDGenerator.cpp:33
+  case AccessSpecifier::AS_none:
+return "";
+  }

return {};


https://reviews.llvm.org/D43424



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

[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

GCC has -Wignored-qualifiers for long time, so may be better to implement it in 
Clang?




Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:66
+  llvm::Optional Loc = findConstToRemove(Def, Result);
+  if (!Loc) return;
+  DiagnosticBuilder Diagnostics = diag(*Loc,

Please split in two lines.



Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:74
+  // associate all fixes with the definition.
+  for (auto *Decl = Def->getPreviousDecl(); Decl != nullptr;
+   Decl = Decl->getPreviousDecl()) {

Please don't use auto, because type is not deducible from statement and it's 
not iterator over container.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`readability-const-value-return
+  ` check.

Please use alphabetical order for list of new checks.



Comment at: docs/ReleaseNotes.rst:64
+  Checks for functions with a ``const``-qualified return type and recommends
+  removal of the `const` keyword.  Such use of ``const`` is superfluous, and
+  prevents valuable compiler optimizations.

Please enclose const in ``.

Once sentence is enough for Release Notes.



Comment at: docs/clang-tidy/checks/readability-const-value-return.rst:7
+Checks for functions with a ``const``-qualified return type and recommends
+removal of the `const` keyword.  Such use of ``const`` is superfluous, and
+prevents valuable compiler optimizations.

Please enclose const in ``.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025



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

[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/readability-const-value-return.rst:7
+Checks for functions with a ``const``-qualified return type and recommends
+removal of the ``const`` keyword.  Such use of ``const`` is superfluous, and
+prevents valuable compiler optimizations.

Please remove the double space


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025



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

[PATCH] D53170: [clang-doc] Switch to default to all-TUs executor

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:205
+   llvm::cl::OptionCategory &Category) {
+  auto OptionsParser =
+  CommonOptionsParser::create(argc, argv, Category, llvm::cl::ZeroOrMore);

Please don't use auto when type is not spelled in same statement.



Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:209
+return OptionsParser.takeError();
+  for (auto I = ToolExecutorPluginRegistry::begin(),
+E = ToolExecutorPluginRegistry::end();

Please use range loop.


https://reviews.llvm.org/D53170



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

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:113
+
+- New alias :doc:`cppcoreguidelines-non-private-member-variables-in-classes
+  
`

Please move new alias after new checks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52771



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/misc/IncorrectPointerCastCheck.cpp:80
+  }
+} // namespace misc
+

Somehow, misc namespace is closed twice.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`misc-incorrect-pointer-cast
+  ` check

Will be good idea to rebase from trunk and use alphabetical order.



Comment at: docs/ReleaseNotes.rst:63
+
+  Warn for cases when pointer is cast and the pointed to type is
+  incompatible with allocated memory area type. This may lead to access memory

Warns. Please also use as much as possible from 80 characters.



Comment at: docs/clang-tidy/checks/misc-incorrect-pointer-cast.rst:10
+
+Warn for cases when the pointed to type is wider than the allocated type. 
+For example char vs integer, long vs char etc.

Looks like two paragraphs are almost same. Will be good idea to rephrase. 
Please make sure that lines are no longer then 80 characters.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48866



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

[PATCH] D53339: Add the abseil-duration-factory-float clang-tidy check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:24
+// Returns an integer if the fractional part of a `FloatingLiteral` is 0.
+llvm::Optional
+TruncateIfIntegral(const FloatingLiteral &FloatLiteral) {

Please use static keyword instead of anonymous namespace.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:70
+  const Expr *Arg = MatchedCall->getArg(0)->IgnoreImpCasts();
+  // We don't mess with macros.
+  if (Arg->getBeginLoc().isMacroID())

May be //Macros are ignored// will sound better?



Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`abseil-duration-factory-float
+  ` check.

Please sort new check lists alphabetically.



Comment at: docs/clang-tidy/checks/abseil-duration-factory-float.rst:8
+``absl::Seconds`` or ``absl::Hours``) are providing a floating point value
+when an integer could be used instead.  The integer factory function overload
+is more efficient and readable.

Please fix double space.


https://reviews.llvm.org/D53339



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

[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

By the word, why this check could not be generalized to any function/method 
which have floating-point and integer variants?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53339



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

[PATCH] D53382: [clang-doc] Update documentation

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Will be good idea to mention improvements in Release Notes.




Comment at: clang-tools-extra/docs/clang-doc.rst:20
 Use
-=
+
 

Isn't it should be same length as heading? Same in other places.


https://reviews.llvm.org/D53382



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

[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:54
+
+namespace {
+

lebedev.ri wrote:
> Extend the namespace /\ above, so that function is also covered?
Anonymous namespaces are only for class/struct declarations. See LLVM coding 
guidelines.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025



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

[PATCH] D53771: [clang-tidy] Avoid C arrays check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

May be this check belongs to modernize?




Comment at: docs/clang-tidy/checks/misc-avoid-c-arrays.rst:6
+
+Find C-style array delarations and recommend to use ``std::array<>``.
+All types of C arrays are diagnosed.

Finds.



Comment at: docs/clang-tidy/checks/misc-avoid-c-arrays.rst:9
+
+Hoever, no fix-its are provided. Such transform would need to be able to
+observe *all* the uses of said declaration in order to decide whether it is

However



Comment at: docs/clang-tidy/checks/misc-avoid-c-arrays.rst:30
+
+  ...
+

Please remove ellipsis. and newline.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53771



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

[PATCH] D51949: [clang-tidy] new check 'readability-isolate-declaration'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:142
+
+  Diagnoses local variable declarations declaring more than one variable and
+  tries to refactor the code to one statement per declaration.

May be Finds or Detects like other checks? Same in documentation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949



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

[PATCH] D53771: [clang-tidy] Avoid C arrays check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Will be good idea to add aliases at least for existing modules.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53771



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

[PATCH] D53771: [clang-tidy] Avoid C arrays check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst:6
+cppcoreguidelines-avoid-c-arrays
+=
+

Please make same length as header.



Comment at: docs/clang-tidy/checks/hicpp-avoid-c-arrays.rst:8
+
+The hicpp-avoid-c-arrays check is an alias,
+please see

Please fill as much as possible to 80 character. Probably same for other alias.



Comment at: docs/clang-tidy/checks/modernize-avoid-c-arrays.rst:6
+
+`cppcoreguidelines-avoid-c-arrays` redirects here
+as an alias for this check.

Please fill as much as possible to 80 character. Same for other alias.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53771



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:22
+ast_matchers::MatchFinder *Finder) {
+  // For the arithmetic calls, we match only the uses of the templated 
operators
+  // where the template parameter is not a built-in type. This means the

Please add check for C++. See other Abseil checks code as example.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:136
+
+  auto SourceRange = Lexer::makeFileCharRange(
+  CharSourceRange::getTokenRange(ArgExpr->getSourceRange()),

Please don't use auto, since return type is not obvious.



Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`abseil-upgrade-duration-conversions
+  ` check.

Please sort new checks list alphabetically.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53830



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

[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/readability-identifier-naming.rst:34
+
+When defined, the checker will ensure abstract class names conform to the
+selected casing.

MyDeveloperDay wrote:
> Eugene.Zelenko wrote:
> > check, please. Same in other places.
> I assumed you mean replace "ensure" with "check", now I'm not sure do you 
> mean?
> 
> A) "the checker will check"
> B) "the check will ensure"
> C) "the check will check"
> 
I'm sorry for been ambiguous. I meant replacing checker with check.  Actually 
this wider problem: //check// is in Clang-tidy; //checker// - in Static Code 
Analyzer.


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

https://reviews.llvm.org/D56563



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I would suggest to rename //contribution// to //Contributing// (see LLVM 
documentation) and //integrations// to //Integrations//.

Will be also good idea to fix D55523  script 
warnings.


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

https://reviews.llvm.org/D54945



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be worth to mention change in Release Notes (in changes list, in 
alphabetical order).


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56644



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D54945#1355920 , @MarinaKalashina 
wrote:

> @Eugene.Zelenko
>
> > I would suggest to rename contribution to Contributing (see LLVM 
> > documentation) and integrations to Integrations.
>
> Sure, done.
>
> > Will be also good idea to fix D55523  
> > script warnings.
>
> The script warns about double spaces and line width in the table, for example:
>
>   warning: line 39 contains double spaces
>   warning: line 39 is in excess of 80 characters (196)
>
>
> These spaces and width are intended since they 'draw' the table layout. Do 
> you think we could ignore such warnings in this particular case?


Sure, it's fine to leave table as it is. Same thing is done in LLDB code where 
tables are excluded from Clang-format.


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

https://reviews.llvm.org/D54945



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

clang-tools-sphinx-docs bot is failing because of:

Warning, treated as error:
/home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/src/tools/clang/tools/extra/docs/clang-tidy/Contributing.rst:61:
 ERROR: Unknown target name: "how to setup tooling for llvm".

Frankly I don't know what is proper way to handle cross-module documentation 
links.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54945



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I fixed links.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54945



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

By the word, I noticed that HTTP was used and replaced it with HTTPS in 
Contributing.rst. Will be good idea to do the same in other documentation.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54945



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