[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:61
+  // Check if any of the superclasses of the class match.
+  for (const auto *SuperClass = Node.getClassInterface()->getSuperClass();
+   SuperClass != nullptr; SuperClass = SuperClass->getSuperClass()) {

Please don't use auto where type is not obvious.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:110
+  Finds invocations of -self on super instances in initializers of subclasses
+  of NSObject and recommends calling a superclass initializer instead.
+

Please enclose NSObject in ``. Probably same for -self if this is language 
construct.



Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst:6
+
+Finds invocations of -self on super instances (`[super self]`) in initializers
+of subclasses of NSObject and recommends invoking a superclass initializer

Please synchronize with Release Notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59806



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:17
 
+using llvm::SmallSet;
+

It'll be easier to use llvm::SmallSet in using statement.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
+
+  Finds cases of `cast<>` used as condition in conditional statements
+  which will assert on failure in Debug builds.

Please use double, not single `.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:23
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+typedef SmallSet HeaderFileExtensionsSet;
 

Please use using. See modernize-use-using.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
+
+  Finds cases of cast<> used as condition in conditional statements which will
+  assert on failure in Debug builds.

Please enclose cast<> in ``.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:6
+
+Finds cases of cast<> used as the condition of a conditional
+statements which will assert on failure in Debug builds.  The use of

Please enclose cast<> in ``. Same below.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:7
+Finds cases of cast<> used as the condition of a conditional
+statements which will assert on failure in Debug builds.  The use of
+cast<> implies that the operation cannot fail, and should not be used

Please fix double space.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:9
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement..
+

Please fix double dot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:26
+llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl ) {
+  if (auto *Func = llvm::dyn_cast()) {

ilya-biryukov wrote:
> Eugene.Zelenko wrote:
> > Functions should be static, not in anonymous namespace. See LLVM Coding 
> > Guidelines.
> We tend to put internal functions to anonymous namespace quite a lot in 
> clangd.
> While technically a small violation of the LLVM style guide, this aligns with 
> the rest of the code and we don't consider that to be a problem.
I don't think it's reasonable to have one style per project. Even if clangd has 
such historical code, it'll be good idea to change it to confirm to common 
guidelines. Evolution of LLDB formatting style is good example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:26
+llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl ) {
+  if (auto *Func = llvm::dyn_cast()) {

Functions should be static, not in anonymous namespace. See LLVM Coding 
Guidelines.



Comment at: clang-tools-extra/clangd/AST.cpp:28
+  if (auto *Func = llvm::dyn_cast()) {
+if (auto *Args = Func->getTemplateSpecializationArgsAsWritten())
+  return Args->arguments();

Return type is not obvious, so auto should not be used.



Comment at: clang-tools-extra/clangd/AST.cpp:131
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+printTemplateArgumentList(OS, *Args, Policy);

Return type is not obvious, so auto should not be used.



Comment at: clang/lib/AST/TypePrinter.cpp:1642
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const auto  = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&

Return type is not obvious, so auto should not be used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59605: [clangd] Introduce background-indexer

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clangd/index/Background.cpp:29
 #include "llvm/Support/SHA1.h"
 
 #include 

Unnecessary empty line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59605



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

[PATCH] D59599: [clangd] Fix a crash while printing Template Arguments

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clangd/AST.cpp:86
   else if (auto *Cls = llvm::dyn_cast()) {
-if (auto STL = Cls->getTypeAsWritten()
-   ->getTypeLoc()
-   .getAs()) {
-  llvm::SmallVector ArgLocs;
-  ArgLocs.reserve(STL.getNumArgs());
-  for (unsigned I = 0; I < STL.getNumArgs(); ++I)
-ArgLocs.push_back(STL.getArgLoc(I));
-  printTemplateArgumentList(OS, ArgLocs, Policy);
+if (auto *TSI = Cls->getTypeAsWritten()) {
+  if (auto STL = TSI->getTypeLoc().getAs()) 
{

Return type is not obvious, so auto should not be used.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59599



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

[PATCH] D59605: [clangd] Introduce background-indexer

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:13
+
+#include 
"/usr/local/google/home/kadircet/repos/llvm/clang-tools-extra/clangd/Context.h"
+#include 
"/usr/local/google/home/kadircet/repos/llvm/clang-tools-extra/clangd/Logger.h"

Please use relative path. Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59605



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

[PATCH] D59540: [clang-tidy] [PR41119] readability-identifier-naming incorrectly fixes lambda capture

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:790
   if (const auto *DeclRef = Result.Nodes.getNodeAs("declRef")) {
-SourceRange Range = DeclRef->getNameInfo().getSourceRange();
-addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
- Result.SourceManager);
-return;
+const auto  = Result.Context->getParents(*DeclRef);
+bool LambdaDeclRef = false;

Type is not obvious, so auto should not be used.



Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:797
+  if (ST && isa(ST)) {
+const auto  = Result.Context->getParents(*ST);
+if (!CastParents.empty())

Type is not obvious, so auto should not be used.


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

https://reviews.llvm.org/D59540



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

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp:197
+
+  const auto Ops = utils::options::parseStringList(CheckedFunctions);
+  Finder->addMatcher(

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


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59103



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

[PATCH] D58977: [clang-tidy] Add the abseil-time-comparison check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-time-comparison.rst:6
+
+Checks for comparisons which should be in the ``absl::Time`` domain instead
+of the integer domains.

Please synchronize with Release Notes.



Comment at: docs/clang-tidy/checks/abseil-time-comparison.rst:10
+N.B.: In cases where an ``absl::Time`` is being converted to an integer,
+alignment may occur.  If the comparison depends on this alingment, doing the
+comparison in the ``absl::Time`` domain may yield a different result.  In

Please fix double spaces. Same below.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58977



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

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:29
+
+DeclRefExpr *findDeclRefExpr(const CXXMemberCallExpr *MemberCallExpr) {
+  auto *ObjectExpr = MemberCallExpr->getImplicitObjectArgument();

Please use static for functions instead of anonymous namespaces. See LLVM 
coding guidelines.



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:30
+DeclRefExpr *findDeclRefExpr(const CXXMemberCallExpr *MemberCallExpr) {
+  auto *ObjectExpr = MemberCallExpr->getImplicitObjectArgument();
+  if (auto *ObjectCast = dyn_cast(ObjectExpr)) {

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



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:44
+void UseRaiiLocksCheck::registerMatchers(MatchFinder *Finder) {
+
+  if (!getLangOpts().CPlusPlus)

Unnecessary empty line.



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:72
+void UseRaiiLocksCheck::check(const MatchFinder::MatchResult ) {
+
+  const auto *LockCallExpr = Result.Nodes.getNodeAs("lock");

Unnecessary empty line.



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:77
+
+  const auto *LockDeclRef = findDeclRefExpr(LockCallExpr);
+  const auto *UnlockDeclRef = findDeclRefExpr(UnlockCallExpr);

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



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:78
+  const auto *LockDeclRef = findDeclRefExpr(LockCallExpr);
+  const auto *UnlockDeclRef = findDeclRefExpr(UnlockCallExpr);
+

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



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:82
+
+  auto LockObjectName = LockDeclRef->getFoundDecl()->getName();
+  auto UnlockObjectName = UnlockDeclRef->getFoundDecl()->getName();

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



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:83
+  auto LockObjectName = LockDeclRef->getFoundDecl()->getName();
+  auto UnlockObjectName = UnlockDeclRef->getFoundDecl()->getName();
+

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



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:89
+
+  auto LockLocation =
+  Result.SourceManager->getPresumedLoc(LockCallExpr->getBeginLoc());

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



Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:91
+  Result.SourceManager->getPresumedLoc(LockCallExpr->getBeginLoc());
+  auto UnlockLocation =
+  Result.SourceManager->getPresumedLoc(UnlockCallExpr->getBeginLoc());

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



Comment at: docs/ReleaseNotes.rst:97
+
+  Checks for explicit usage of``std::mutex`` functions ``lock()``, 
+  ``try_lock()`` and ``unlock()``.

Missing space before ``std::mutex



Comment at: docs/clang-tidy/checks/cppcoreguidelines-use-raii-locks.rst:6
+
+The explicit use of ``std::mutex`` functions ``lock()``, ``try_lock()`` and 
+``unlock()`` is error prone and should be avoided. 

Please synchronize first statement with Release Notes.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-use-raii-locks.rst:18
+   locking and unlocking a mutex.
+   Defaults to: ``::std::mutex;::std::recursive_mutex;::std::timed_mutex;
+   ::std::recursive_timed_mutex;::std::unique_lock``.

Please use single ` for options values.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58818



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

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-override.rst:5
 ==
 
 

Will be good idea to remove duplicated empty line.



Comment at: docs/clang-tidy/checks/modernize-use-override.rst:14
+
+   If set to non-zero, this check will not diagnose destructors. Default is 
'0'.

Please use ` to highlight value.


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

https://reviews.llvm.org/D58731



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang/include/clang/Index/DeclOccurrence.h:1
+//===--- DeclOccurrence.h - An occurrence of a decl within a file - C++ 
-*-===//
+//

Tag should contain *- C++ -*, so you could remove dashes at beginning and equal 
signs to fit.


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

https://reviews.llvm.org/D58478



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang/include/clang/Index/DeclOccurrence.h:1
+//===--- DeclOccurrence.h - An occurrence of a decl within a file 
-===//
+//

Missing C++ tag. See other headers as example.



Comment at: clang/include/clang/Index/DeclOccurrence.h:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.

License should be updated.



Comment at: clang/include/clang/Index/DeclOccurrence.h:42
+
+#endif

Missing comment with header guard.



Comment at: clang/lib/Index/FileIndexRecord.cpp:45
+  for (auto  : Decls) {
+auto D = DclInfo.Dcl;
+SourceManager  = D->getASTContext().getSourceManager();

Type is not obvious, so auto should not be used.



Comment at: clang/lib/Index/FileIndexRecord.h:1
+//===--- FileIndexRecord.h - Index data per file 
--===//
+//

Missing C++ tag. See other headers as example.



Comment at: clang/lib/Index/FileIndexRecord.h:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.

License should be updated.



Comment at: clang/lib/Index/FileIndexRecord.h:58
+
+#endif

Missing comment with header guard.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58478



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

[PATCH] D58137: [clang-tidy] Add the abseil-time-subtraction check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:72
+
+// Match the cases where we know that the result is a Duration and the 
first
+// argument is a Time.  Just knowing the type of the first operand is not

JonasToth wrote:
> please highlight the types with ''
But do we do this in comments? Isn't it's Sphinx syntax?


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

https://reviews.llvm.org/D58137



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

[PATCH] D58137: [clang-tidy] Add the abseil-time-subtraction check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:91
+
+  Finds and fixes `absl::Time` subtraction expressions to do subtraction
+  in the Time domain instead of the numeric domain.

Please use two not one ` for language constructs.



Comment at: docs/clang-tidy/checks/abseil-time-subtraction.rst:11
+information:
+ - When the result is a ``Duration`` and the first argument is a ``Time``.
+ - When the second argument is a ``Time``.

Will be good idea to always use //absl::// prefix. Same in other places.



Comment at: docs/clang-tidy/checks/abseil-time-subtraction.rst:25
+
+  // Original - `Duration` result and first operand is a `Time`.
+  absl::Duration d = absl::Seconds(absl::ToUnixSeconds(t) - x);

` doesn't make sense in comments. Same below.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58137



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://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 new check in Release Notes.




Comment at: clang-tidy/readability/RedundantExternCheck.cpp:55
+} // namespace clang
\ No newline at end of file


Please add new line.



Comment at: clang-tidy/readability/RedundantExternCheck.h:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.

Please update license statement. See current Clang-tidy code as example. Same 
in other files.



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

Please remove //This checker//. Language constructs should be enclosed in ``.


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

https://reviews.llvm.org/D33841



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

[PATCH] D57966: [clang-tidy] add camelBackOrCase casing style to readability-identifier-naming to support change to variable naming policy (if adopted)

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/readability-identifier-naming.rst:30
 
+``camelBackOrCase`` allows for both `camelBack` or `CamelCase`  cased
+identifiers to be used to match prior LLVM code style

Please fix double space.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57966



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

[PATCH] D57674: [clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:82
 
+- The :doc:`bugprone-argument-comment
+  ` now supports 

Please move changes in existing checks after list of new checks.


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

https://reviews.llvm.org/D57674



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

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.
Herald added subscribers: dkrupp, rnkovacs.
Herald added a project: LLVM.

Please look at recently filed PR40544.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D35937



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:80
+  if (cons) {
+if (cons->isListInitialization()) {
+  return;

Please elide braces.


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

https://reviews.llvm.org/D57435



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:61
+"abseil-wrap-unique");
+ 
+ }

Unnecessary empty line.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:10
+
+#include 
+#include "WrapUniqueCheck.h"

Please run Clang-format.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:75
+  if(cons){
+   
+if (cons->isListInitialization()){

Unnecessary empty line.



Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`abseil-wrap-unique
+  ` check.

Please use alphabetical order in new checks list.



Comment at: docs/ReleaseNotes.rst:74
+  Looks for instances of factory functions which uses a non-public constructor
+  that returns a std::unqiue_ptr then recommends using 
+  absl::wrap_unique(new T(...)).

Please use `` to highlight language constructs. Same in documentation.



Comment at: docs/clang-tidy/checks/abseil-wrap-unique.rst:6
+
+Checks for instances of static function within a class being called and
+returning a std:unique_ptr type. Also checks for instances where reset

Please synchronize first statement with Release Notes.



Comment at: docs/clang-tidy/checks/abseil-wrap-unique.rst:24
+ 
+  //Original - reset called with a static function returning a std::unqiue_ptr
+  a.reset(A::NewA());

Please run Clang-format over code snippets.



Comment at: test/clang-tidy/abseil-wrap-unique.cpp:3
+
+
+namespace std {

Unnecessary empty line.



Comment at: test/clang-tidy/abseil-wrap-unique.cpp:31
+}  // namespace std
+
+

Unnecessary empty line.



Comment at: test/clang-tidy/abseil-wrap-unique.cpp:88
+  //std::unique_ptr e(new int[2] {1,2});
+  
+}

Unnecessary empty line.



Comment at: test/clang-tidy/abseil-wrap-unique.cpp:90
+}
+
+

Unnecessary empty lines.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D57353: [clang-tidy] Add the abseil-duration-double-conversion check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-double-conversion.rst:28
+
+Note: Converting to an integer and back to an `absl::Duration` might be a
+truncating operation if the value is not aligned to the scale of conversion.

Please use `` for language constructs. Same below.


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

https://reviews.llvm.org/D57353



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

[PATCH] D57353: [clang-tidy] Add the abseil-duration-double-conversion check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/DurationDoubleConversionCheck.cpp:48
+
+  if (!isNotInMacro(Result, OuterCall)) return;
+

Please run Clang-format.



Comment at: docs/ReleaseNotes.rst:85
+
+  Find and fix cases where `absl::Duration` values are being converted to
+  numeric types and back again.

Finds and fixes. Please use `` for language constructs. Same in documentation.



Comment at: docs/clang-tidy/checks/abseil-duration-double-conversion.rst:20
+
+
+  // Original - Conversion to integer and back again

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/abseil-duration-double-conversion.rst:27
+  absl::Duration d2 = d1;
+
+

Unnecessary empty line.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57353



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

[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new module and a check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

New module is still not mentioned in docs/clang-tidy/index.rst.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113



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

[PATCH] D57185: [clang-tidy] Add the abseil-duration-addition check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:76
 
+- New :doc:`abseil-duration-addition
+  ` check.

Please use alphabetical order for new checks.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57185



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

[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new module and a check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new module in docs/clang-tidy/index.rst.




Comment at: docs/ReleaseNotes.rst:76
 
+- New OpenMP module.
+

Please put new module before list of new checks.



Comment at: docs/ReleaseNotes.rst:83
+
+  Diagnoses directives that are allowed to contain 'default(none)' clause,
+  but don't contain it.

Please use `` for default(none). Will be good idea to make this statement same 
as in documentation.



Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:19
+
+
+  // 'for' directive can not have 'default' clause, no diagnostics.

Unnecessary empty line.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113



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

[PATCH] D56946: [Documentation] Use HTTPS whenever possible in Clang

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Yes, I tested all URLs, but I think independent testing will not be excessive.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56946



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

[PATCH] D56945: [clang-tidy] Delete obsolete objc-property-declaration options ✂️

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:248
 
+- The `Acronyms` and `IncludeDefaultAcronyms` options for the
+  :doc:`objc-property-declaration`

Please rebase from trunk and use :option: prefix for options..



Comment at: docs/ReleaseNotes.rst:249
+- The `Acronyms` and `IncludeDefaultAcronyms` options for the
+  :doc:`objc-property-declaration`
+  check have been removed.

Please add space before <.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56945



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

[PATCH] D56946: [Documentation] Use HTTPS whenever possible in Clang

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko created this revision.
Eugene.Zelenko added reviewers: sylvestre.ledru, hans.
Eugene.Zelenko added a project: clang.

Repository:
  rC Clang

https://reviews.llvm.org/D56946

Files:
  llvm-svn.src/tools/clang/docs/AutomaticReferenceCounting.rst
  llvm-svn.src/tools/clang/docs/ClangFormatStyleOptions.rst
  llvm-svn.src/tools/clang/docs/ControlFlowIntegrity.rst
  llvm-svn.src/tools/clang/docs/ControlFlowIntegrityDesign.rst
  llvm-svn.src/tools/clang/docs/ExternalClangExamples.rst
  llvm-svn.src/tools/clang/docs/HardwareAssistedAddressSanitizerDesign.rst
  llvm-svn.src/tools/clang/docs/HowToSetupToolingForLLVM.rst
  llvm-svn.src/tools/clang/docs/InternalsManual.rst
  llvm-svn.src/tools/clang/docs/IntroductionToTheClangAST.rst
  llvm-svn.src/tools/clang/docs/JSONCompilationDatabase.rst
  llvm-svn.src/tools/clang/docs/LanguageExtensions.rst
  llvm-svn.src/tools/clang/docs/MSVCCompatibility.rst
  llvm-svn.src/tools/clang/docs/PCHInternals.rst
  llvm-svn.src/tools/clang/docs/SafeStack.rst
  llvm-svn.src/tools/clang/docs/SanitizerCoverage.rst
  llvm-svn.src/tools/clang/docs/Toolchain.rst
  llvm-svn.src/tools/clang/docs/UndefinedBehaviorSanitizer.rst
  llvm-svn.src/tools/clang/docs/UsersManual.rst
  llvm-svn.src/tools/clang/docs/analyzer/DesignDiscussions/InitializerLists.rst

Index: llvm-svn.src/tools/clang/docs/analyzer/DesignDiscussions/InitializerLists.rst
===
--- llvm-svn.src/tools/clang/docs/analyzer/DesignDiscussions/InitializerLists.rst
+++ llvm-svn.src/tools/clang/docs/analyzer/DesignDiscussions/InitializerLists.rst
@@ -145,7 +145,7 @@
 actual metadata value. So we'd be escaping more stuff than necessary.
 
 If only we had "ghost fields"
-(http://lists.llvm.org/pipermail/cfe-dev/2016-May/049000.html), it would have
+(https://lists.llvm.org/pipermail/cfe-dev/2016-May/049000.html), it would have
 been much easier, because the ghost field would only contain the actual
 metadata, and the Store would always know about it. This example adds to my
 belief that ghost fields are exactly what we need for most C++ checkers.
Index: llvm-svn.src/tools/clang/docs/UsersManual.rst
===
--- llvm-svn.src/tools/clang/docs/UsersManual.rst
+++ llvm-svn.src/tools/clang/docs/UsersManual.rst
@@ -15,8 +15,8 @@
 these languages. Clang builds on the LLVM optimizer and code generator,
 allowing it to provide high-quality optimization and code generation
 support for many targets. For more general information, please see the
-`Clang Web Site `_ or the `LLVM Web
-Site `_.
+`Clang Web Site `_ or the `LLVM Web
+Site `_.
 
 This document describes important notes about using Clang as a compiler
 for an end-user, documenting the supported features, command line
@@ -994,7 +994,7 @@
 Precompiled Headers
 ---
 
-`Precompiled headers `__
+`Precompiled headers `_
 are a general approach employed by many compilers to reduce compilation
 time. The underlying motivation of the approach is that it is common for
 the same (and often large) header files to be included by multiple
@@ -1482,7 +1482,7 @@
 
 3. Convert the collected profile data to LLVM's sample profile format.
This is currently supported via the AutoFDO converter ``create_llvm_prof``.
-   It is available at http://github.com/google/autofdo. Once built and
+   It is available at https://github.com/google/autofdo. Once built and
installed, you can convert the ``perf.data`` file to LLVM using
the command:
 
@@ -1521,12 +1521,12 @@
 
 2. Binary encoding. This uses a more efficient encoding that yields smaller
profile files. This is the format generated by the ``create_llvm_prof`` tool
-   in http://github.com/google/autofdo.
+   in https://github.com/google/autofdo.
 
 3. GCC encoding. This is based on the gcov format, which is accepted by GCC. It
is only interesting in environments where GCC and Clang co-exist. This
encoding is only generated by the ``create_gcov`` tool in
-   http://github.com/google/autofdo. It can be read by LLVM and
+   https://github.com/google/autofdo. It can be read by LLVM and
``llvm-profdata``, but it cannot be generated by either.
 
 If you are using Linux Perf to generate sampling profiles, you can use the
@@ -1831,9 +1831,9 @@
 
 where ``fragmentkind`` is one of ``name``, ``type``, or ``encoding``,
 indicating whether the following mangled name fragments are
-<`name `_>s,
-<`type `_>s, or
-<`encoding `_>s,
+<`name `_>s,
+<`type 

[PATCH] D56926: [Documentation] Use HTTPS whenever possible in clang-tools-extra

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko updated this revision to Diff 182590.
Eugene.Zelenko added a comment.

Don't use HTTPS for www.codingstandard.com


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56926

Files:
  llvm-svn.src/tools/clang/tools/extra/docs/clang-doc.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-rename.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/Integrations.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/llvm-include-order.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/llvm-namespace-comment.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-pass-by-value.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-use-emplace.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/portability-simd-intrinsics.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/readability-else-after-return.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/readability-magic-numbers.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/index.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clangd.rst
  llvm-svn.src/tools/clang/tools/extra/docs/include-fixer.rst
  llvm-svn.src/tools/clang/tools/extra/docs/modularize.rst
  llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst

Index: llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst
===
--- llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst
+++ llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst
@@ -11,7 +11,7 @@
 activity. It's also used as a test of Clang's PPCallbacks interface.
 It runs a given source file through the Clang preprocessor, displaying
 selected information from callback functions overridden in a
-`PPCallbacks `_
+`PPCallbacks `_
 derivation. The output is in a high-level YAML format, described in
 :ref:`OutputFormat`.
 
@@ -32,7 +32,7 @@
  specifies the source file to run through the preprocessor.
 
  is a place-holder for regular
-`Clang Compiler Options `_,
+`Clang Compiler Options `_,
 which must follow the .
 
 .. _CommandLineOptions:
@@ -88,7 +88,7 @@
 pp-trace Output Format
 ==
 
-The pp-trace output is formatted as YAML. See http://yaml.org/ for general
+The pp-trace output is formatted as YAML. See https://yaml.org/ for general
 YAML information. It's arranged as a sequence of information about the
 callback call, including the callback name and argument information, for
 example:::
@@ -150,8 +150,8 @@
 value, only some key member or members are shown to represent the value,
 instead of trying to display all members of the structure.
 
-`FileChanged `_ Callback
-
+`FileChanged `_ Callback
+^
 
 FileChanged is called when the preprocessor enters or exits a file, both the
 top level file being compiled, as well as any #include directives. It will
@@ -177,8 +177,8 @@
 FileType: C_User
 PrevFID: (invalid)
 
-`FileSkipped `_ Callback
-
+`FileSkipped `_ Callback
+^
 
 FileSkipped is called when a source file is skipped as the result of header
 guard optimization.
@@ -200,8 +200,8 @@
 FilenameTok: "filename.h"
 FileType: C_User
 
-`FileNotFound `_ Callback
-^
+`FileNotFound 

[PATCH] D56926: [Documentation] Use HTTPS whenever possible in clang-tools-extra

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko created this revision.
Eugene.Zelenko added reviewers: alexfh, aaron.ballman, JonasToth, 
MyDeveloperDay, juliehockett, lebedev.ri.
Eugene.Zelenko added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, kbarton, nemanjai.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56926

Files:
  llvm-svn.src/tools/clang/tools/extra/docs/clang-doc.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-rename.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/Integrations.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/google-objc-global-variable-declaration.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-avoid-goto.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-braces-around-statements.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-deprecated-headers.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-exception-baseclass.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-explicit-conversions.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-function-size.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-invalid-access-moved.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-member-init.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-move-const-arg.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-named-parameter.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-new-delete-operators.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-no-array-decay.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-no-assembler.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-no-malloc.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-noexcept-move.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-special-member-functions.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-static-assert.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-undelegated-constructor.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-auto.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-emplace.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-equals-default.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-equals-delete.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-noexcept.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-nullptr.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-use-override.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/hicpp-vararg.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/llvm-include-order.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/llvm-namespace-comment.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-pass-by-value.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-use-emplace.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/portability-simd-intrinsics.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/readability-else-after-return.rst
  
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/readability-magic-numbers.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/index.rst
  llvm-svn.src/tools/clang/tools/extra/docs/clangd.rst
  llvm-svn.src/tools/clang/tools/extra/docs/include-fixer.rst
  llvm-svn.src/tools/clang/tools/extra/docs/modularize.rst
  llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst

Index: llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst
===
--- llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst
+++ llvm-svn.src/tools/clang/tools/extra/docs/pp-trace.rst
@@ -11,7 +11,7 @@
 activity. It's also used as a test of Clang's PPCallbacks interface.
 It runs a given source file through the Clang preprocessor, displaying
 selected information from callback functions overridden in a
-`PPCallbacks `_
+`PPCallbacks `_
 derivation. The output is in a 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I requested merge of updated documentation into 8.0 branch in PR40369.


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

[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.

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] 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.

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] 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] D56532: [clang-tidy] Add the abseil-duration-conversion-cast check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/DurationConversionCastCheck.cpp:41
+
+  if (!isNotInMacro(Result, MatchedCast)) return;
+

Please run Clang-format.


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

https://reviews.llvm.org/D56532



___
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 inline comments.



Comment at: docs/clang-tidy/integrations.rst:118
+allows to use a custom :program:`clang-tidy` binary.
\ No newline at end of file


Please add newline.


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] D56424: [clang-tidy] Add check for underscores in googletest names.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:161
+
+  Checks that Googletest test and test case names do not contain an underscore,
+  which is prohibited by the Googletest FAQ.

Please synchronize with first statement in documentation.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:46
+auto IdentifierInfo = MacroNameToken.getIdentifierInfo();
+if (!IdentifierInfo) {
+  return;

Please elide braces.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:45
+std::string(Message) +
+" (no FixIt provided, function argument list end is inside 
macro)");
+return {};

MyDeveloperDay wrote:
> bernhardmgruber wrote:
> > JonasToth wrote:
> > > I think you can ellide that extra message. Not emitting the fixit is 
> > > clear already.
> > I actually like having a reason why my check could not provide a FixIt. 
> > Especially because there are multiple reasons why this might happen.
> @bernhardmgruber I had the same comment given to me on a review recently with 
> regard to diag message, let me try and help you with what I though was the 
> rational... I think the premise is something like:
> 
> 1) "no FixIt provided" is implied by the fact it isn't fixed
> 2) "function type source info is missing"  doesn't tell the developer what 
> they have to do to have it be fixed
> 
> sure it helps you as the checker developer but probably that isn't much use 
> to a developer using the checker on their code and so might confuse them.
> 
> It also makes grepping for messages in a log file difficult because it means 
> multiple messages coming from your checker have a different pattern (although 
> there might be a common sub pattern)
> 
> For the most part where a fixit is not supplied where it should someone would 
> create a test case which you could consume in your tests
> 
> To be honest as a general observation as a newbie myself, what I've noticed 
> is that a lot of checker review comments are very similar, 
> 
> 
> 
>   - 80 characters in rst files
>   - clang-format
>   - alphabetic order
>   - Comments with proper puncuation
>   - code in comments in ``XXX``
>   - don't overly use auto
>   - don't use anonymous namespace functions use static functions
>   - run it on a big C++ project
>   - run it over all of LLVM
>   - consistency of coding style (elide unnecessary const)
>   - elide unnecessary braces/brackets/code/etc..
> 
> 
> 
> We really should try and write a "Writing a clang checker, and getting it 
> though review" primer, because I really feel for these "gaints" that we ask 
> to review all this code, they must go over the same thing and have to present 
> the same reasons time and time again...
> 
> which is why If you don't mind I'd like to try to help give something back by 
> filling in some of the reasoning gaps here to a fellow new starter
> 
> 
> 
> 
> 
> 
> 
> 
I would say that we should eat own dog food :-)

I'd love to see your documentation validation scripts as part of build!

We also should regularly run Clang-tidy on BuildBot. But first we must fix 
existing warnings and no everybody happy if such cleanups performed by 
outsiders.

See PR27267 for anonymous namespaces usage.

Clang-tidy has modernize-use-auto, but not check for LLVM guidelines 
conformance.

Braces should be checked by readability-braces-around-statements, but proper 
setup is needed.

Conformance to readability-else-after-return is another typical newbies problem.


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

https://reviews.llvm.org/D56160



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

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:35
 
+- New :doc:`modernize-use-trailing-return
+  ` check.

Please rebase from trunk and use alphabetical order for new checks list.


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

https://reviews.llvm.org/D56160



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

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new check in Release Notes and list of checks. It'll be good 
idea to used add_new_check.py.




Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:22
+// very similar to UseOverrideCheck
+SourceLocation findTrailingReturnTypeLocation(const CharSourceRange ,
+  const ASTContext ,

Please use static instead of anonymous namespace. Same for other function. See 
LLVM coding guidelines.


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

https://reviews.llvm.org/D56160



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

[PATCH] D56012: [clang-tidy] Be more liberal about literal zeroes in abseil checks

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: test/clang-tidy/abseil-duration-factory-scale.cpp:3
 
+#include 
 #include "absl/time/time.h"

cstdint, please.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56012



___
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.

Please run script from D55523  over your 
changes.


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] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:36
+   NS != NSEnd; ++NS) {
+const auto *decl = *NS;
+

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


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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please close PR25403 after commit.




Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`readability-duplicated-access-specifiers
+  ` check.

Please use alphabetical order for new checks.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55793



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

[PATCH] D55640: [clang-tidy] Implement a check for large Objective-C type encodings 

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/objc-type-encoding-size.rst:6
+
+Finds Objective-C type encodings that exceed a configured threshold.
+

Please synchronize with Release Notes.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55640



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/MakeUniqueCheck.cpp:28
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasName("::std::unique_ptr"), templateArgumentCountIs(2),
+  hasTemplateArgument(

EricWF wrote:
> Does this catch `std::__1::unique_ptr`, where `__1` is an inline namespace? 
> (Either way, we should add a test)
> 
> Maybe it's better to first filter declarations using an `isInStdNamespace` 
> matcher and then check for the name `unique_ptr`.
Tests for other checks don't contain libc++ specific, so if problems existed 
they were resolved long time ago.


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

https://reviews.llvm.org/D55044



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:93
+
+  Checks for instances of initializing a `unique_ptr` with a direct call to
+  `new` and suggests using `absl::make_unique` instead.

Please use double `, not single ones to hightlight language constructs. Same in 
documentation.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:10
+Replaces initialization of smart pointers:
+\code
+  std::unique_ptr ptr = std::unique_ptr(new int(1));

Please use .. code-block:: c++. See other checks documentation as example.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:19
+
+per the Abseil tips and guidelines at https://abseil.io/tips/126.

Please use hyperlink like:

The `Abseil Style Guide `_ discusses this issue in 
more detail.


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

https://reviews.llvm.org/D55044



___
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: docs/ReleaseNotes.rst:139
+
+  Warns for cases when pointer is cast and the pointed to type is
+  incompatible with allocated memory area type. This may lead to access memory

Please wrap up to 80 characters. Same in documentation.



Comment at: docs/clang-tidy/checks/misc-incorrect-pointer-cast.rst:8
+allocated type.
+For example char vs integer, long vs char etc.
+Also warns for cases when the pointed to type layout is different from the 

Integer -> int. Please highlight types with ``.


Repository:
  rCTE Clang Tools Extra

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

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] D55410: [clang-tidy] check for flagging using declarations in headers

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:13
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases 
discusses this
+issue in more detail

Naysh wrote:
> Eugene.Zelenko wrote:
> > Missing link to guidelines,
> @Eugene.Zelenko: Sorry, I'm not sure what you mean. Could you clarify what 
> guidelines you're referring to? 
Please see documentation in other using-related checks.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55410



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:49
+
+
+.. code-block:: c++

Unnecessary empty line.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

By the word, will be good idea to have script which check alphabetical order 
and use it during build. Sometimes alphabetical order may be violated after 
merge with trunk.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Specify a macro to use instead of ``[[nodiscard]]``.  This is useful when 
+maintaining source code that needs to compile with a pre-C++17 compiler.

Option specif**ies**. Please fix double space.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Specify a macro to use instead of ``[[nodiscard]]``. 
+This is useful when maintaining source code that needs to compile with a 
pre-C++17 compiler.

Specifies. Please wrap up to 80 characters.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Thank you for this fix!


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

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:173
+
+  This check adds ``[[nodiscard]]`` attributes (introduced in C++17) to member
+  functions to highlight at compile time where the return value of a function

Please omit this check. Same in documentation.



Comment at: docs/ReleaseNotes.rst:175
+  functions to highlight at compile time where the return value of a function
+  should not be ignored
+

Please add dot at the end.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:40
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+
+  // if we are using C++17 attributes we are going to need c++17

Unnecessary empty line.



Comment at: docs/ReleaseNotes.rst:173
+
+  Checks to detect if a member function looks like its return type should not 
+  be ignored.

One sentence is enough for Release Notes. Will be good idea to make it same as 
in documentation.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:40
+
+Users can use :option:`ReplacementString` to specify a macro to use instead of
+``[[nodiscard]]``. This is useful when maintaining source code that needs to

May be just this option?


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:21
+namespace modernize {
+namespace {
+

Please use static instead anonymous namespace for functions. See LLVM code 
style guidelines.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:103
+
+  auto retLoc = MatchedDecl->getInnerLocStart();
+

Please don't use auto when type could not be deducted from same statement.



Comment at: docs/ReleaseNotes.rst:76
 
+- New :doc:`modernize-use-nodiscard
+  ` check.

Please use alphabetical order for new checks list.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:6
+
+This check adds ``[[nodiscard]]`` attributes (introduced in C++17) to member 
functions to highlight at compile time where the return value of a function 
should not be ignored
+

Please use 80 characters limit.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55433



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

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:13
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases 
discusses this
+issue in more detail

Missing link to guidelines,


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55410



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

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:73
+
+  Flags using declarations in header files, and suggests that
+  these aliases be removed

Please highlight using with ``.



Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:6
+
+Flags using declarations in header files, and suggests that these aliases be 
removed.
+

Please highlight using with ``. Same in other places.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55410



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

[PATCH] D55346: [clang-tidy] check for using declaration scope and qualification

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst:20
+
+  } // foo

Are there guidelines?



Comment at: docs/clang-tidy/checks/abseil-qualified-aliases.rst:22
+
+See https://abseil.io/tips/119 for an in depth justification for this
+check. 

Please make text consistent across all these checks.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55346



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

[PATCH] D55346: [clang-tidy] check for using declaration scope and qualification

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:31
+
+
+void AnonymousEnclosedAliasesCheck::check(const MatchFinder::MatchResult 
) {

Please remove unnecessary empty line.



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:33
+void AnonymousEnclosedAliasesCheck::check(const MatchFinder::MatchResult 
) {
+  
+  const UsingDecl *MatchedUsingDecl = 

Please remove unnecessary empty line.



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:34
+  
+  const UsingDecl *MatchedUsingDecl = 
+   Result.Nodes.getNodeAs("using_decl");

const auto *



Comment at: clang-tidy/abseil/QualifiedAliasesCheck.cpp:13
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include 

Please remove unnecessary empty line.



Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:27
+usingDecl(hasParent(namespaceDecl(forEach(namespaceDecl() )
+).bind("use"), this);
+}

Please run Clang-format over patch.



Comment at: docs/ReleaseNotes.rst:73
+
+  Flags using declarations in header files, and suggests that 
+  these aliases be removed.

Please enclose using in ``. Same in other places, including documentation.



Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:10
+accept the specified alias. This is bad practice, which is why the check 
suggests
+such declarations be removed. 

Are there guidelines?



Comment at: docs/clang-tidy/checks/abseil-qualified-aliases.rst:24
+check. 
+ 

Please remove unnecessary empty line.



Comment at: docs/clang-tidy/checks/abseil-safely-scoped.rst:28
+See https://abseil.io/tips/119 for more explanation. 
+

Please remove unnecessary empty line.



Comment at: test/clang-tidy/abseil-qualified-aliases.cpp:27
+} // namespace example
+

Please remove unnecessary empty line.



Comment at: test/clang-tidy/abseil-safely-scoped.cpp:27
+}  // namespace foo
+

Please remove unnecessary empty line.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55346



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

[PATCH] D42682: [clang-tidy] Add io-functions-misused checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/bugprone-io-functions.rst:4
+bugprone-io-functions
+=
+

Please adjust length.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D42682



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-subtraction.rst:7
+Checks for cases where subtraction should be performed in the
+``absl::Duration`` domain.  When subtracting two values, and the first one is
+known to be a conversion from ``absl::Duration``, we can infer that the second

Please fix double space.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55245



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please close PR30233 after committing patch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int ) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else 
branches [bugprone-branch-clone]

donat.nagy wrote:
> JonasToth wrote:
> > donat.nagy wrote:
> > > JonasToth wrote:
> > > > could you please add tests for ternary operators?
> > > Currently the check does not handle ternary operators, but I added some 
> > > checks reflecting this.
> > Thank you. Could you please add a short note to the user-facing 
> > documentation that these are not handled.
> In fact, I could easily extend the functionality of the checker to cover 
> ternary operators. Would it be an useful addition?
Sure, it'll be great extension of functionality.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`abseil-make-unique
+  ` check.

Please use alphabetical order for new checks.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:6
+
+Replaces unique pointers that are constructed with raw pointers with 
``absl::make_unique``, for leak-free dynamic allocation.
+

Please synchronize with Release Notes.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55044



___
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 inline comments.



Comment at: docs/clang-tidy/index.rst:304
+CLion_ 2017.2 and later `integrates clang-tidy`_ as an extension to the 
built-in code analyzer.
+Inspections and applicable quick-fixes are performed on the fly, and checks 
can be configured in the standard command line
+format. In this integration, you can switch to the :program:`clang-tidy` 
binary different from the bundled one and pass

Please limit line width to 80 characters. Same in other places.



Comment at: docs/clang-tidy/index.rst:327
+
+`MS Visual Studio`_ can integrate :program:`clang-tidy` by means of three 
different tools.
+The `ReSharper C++`_ extension, version 2017.3 and later, provides seamless 
:program:`clang-tidy` integration:

Will be good idea to mention native plugin, clang-tidy-vs.


Repository:
  rCTE Clang Tools Extra

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] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/utils/LexerUtils.h:43
+  if (Start.isInvalid() || Start.isMacroID())
+return SourceLocation();
   while (true) {

{} could returned instead.



Comment at: docs/ReleaseNotes.rst:180
   removal of the ``const`` keyword.
+>>> master
 

Merge artifact.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst:6
+
+This check implements detection of local variables which could be declared as
+``const``, but are not. Declaring variables as ``const`` is required by many

Will be good idea to synchronize first statement with Release Notes.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54943



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D54757#1305114, @whisperity wrote:

> Bar the previous comments, I really like this. The test suite is massive and 
> well-constructed. Do we know of any real-world findings, maybe even from LLVM?


GCC 7 introduced -Wduplicated-branches, so will be good idea to run this check 
on associated regression(s).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-comparison.rst:11
+compared against a floating-pointer value, truncation during the ``Duration``
+conversion might yield a different result.  In practice this is very rare, and
+still indicates a bug which should be fixed.

Please fix double space.



Comment at: docs/clang-tidy/checks/abseil-duration-comparison.rst:34
+  if (absl::Microseconds(x) < d) ...
+..

Please remove ..


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54737



___
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: docs/ReleaseNotes.rst:60
 
+- New :doc:`misc-incorrect-pointer-cast
+  ` check

hgabii wrote:
> Eugene.Zelenko wrote:
> > Will be good idea to rebase from trunk and use alphabetical order.
> It is rebased. I could not see what should be in an alphabetical order. This 
> is inserted to the top of the document by the //add_new_check.py//.
List of new check should be sorted alphabetically. Unfortunately 
add_new_check.py doesn't do this, so manual edit is necessary.


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] D54757: [clang-tidy] new check: bugprone-branch-clone

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:19
+
+namespace {
+

Please use anonymous namespaces only for type declarations. See LLV coding 
style guide.



Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:43
+
+  for (unsigned i = 0; i < LHS.size(); i++) {
+if (!areStatementsIdentical(LHS[i]->stripLabelLikeStatements(),

Please use size_t.



Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:122
+
+const int N = Branches.size();
+llvm::BitVector KnownAsClone(N);

Please use size_t. Same for loop variables. Clang complains about such 
conversions.



Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`bugprone-branch-clone
+  ` check.

Please use alphabetical order for new checks list.



Comment at: docs/clang-tidy/checks/bugprone-branch-clone.rst:6
+
+Finds ``if/else if/else`` chains where two or more branches are Type I clones
+of each other (that is, they contain identical code) and ``switch`` statements

Please first statement same as Release Notes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-pp'

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:73
+
+  Finds potentially redundant preprocessor usage.
+

preprocessor directives? Same in documentation.



Comment at: docs/clang-tidy/checks/readability-redundant-pp.rst:8
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition.

JonasToth wrote:
> This needs more explanation, please add `.. code-block:: c++` sections for 
> the cases that demonstrate the issue.
Please use ``. Same below.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54349



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clangd/index/Background.cpp:29
 #include 
+#include 
+#include 

Please run Clang-format. Headers in sections should be in alphabetical order.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D54262: [clang-tidy] Add `delete this` bugprone check (PR38741)

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please also add regression test case. Is should also cover standalone function 
with this variable.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54262



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

[PATCH] D54262: [clang-tidy] Add `delete this` bugprone check (PR38741)

[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/bugprone-delete-this.rst:8
+
+Said statement generates multiple problems, such as enforcing allocating the 
object via ``new``, or not allowing any
+usage of instance members (neither fields nor methods) after the execution of 
it, nor touching ``this`` pointer

Please use 80 characters limit.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54262



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

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:88
+  Checks for cases where arguments to ``absl::Duration`` factory functions are
+  scaled internally and could be changed to a different factory function.  This
+  check also looks for arguements with a zero value and suggests using

hwright wrote:
> Eugene.Zelenko wrote:
> > Second statement belongs to documentation.
> It already is in the user-facing documentation, are saying it should be 
> removed from here?
Release Notes are for brief descriptions. Documentation is for details.


https://reviews.llvm.org/D54246



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

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:36
+GetScaleForFactory(llvm::StringRef FactoryName) {
+  static const auto *ScaleMap =
+  new std::unordered_map(

This will cause memory leaks, so may be unique_ptr should be used to hold 
pointer? May be LLVM ADT has better container?



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:59
+return IntLit->getValue().getLimitedValue();
+  } else {
+assert(FloatLit != nullptr);

else after return. Same in other places.



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:164
+  }
+  return "unreachable";
+}

Should be llvm_unreachable().



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:288
+  // We can't do anything with this expression, so return.
+  return;
+}

Please remove trailing return and comment above. See [[ 
http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-control-flow.html
 | readability-redundant-control-flow ]].



Comment at: docs/ReleaseNotes.rst:88
+  Checks for cases where arguments to ``absl::Duration`` factory functions are
+  scaled internally and could be changed to a different factory function.  This
+  check also looks for arguements with a zero value and suggests using

Second statement belongs to documentation.



Comment at: docs/clang-tidy/checks/abseil-duration-factory-scale.rst:7
+Checks for cases where arguments to ``absl::Duration`` factory functions are
+scaled internally and could be changed to a different factory function.  This
+check also looks for arguements with a zero value and suggests using

Please fix double space.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54246



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

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new check in Release Notes (in alphabetical order).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54222



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

[PATCH] D54222: [clang-tidy/checks] Add a checker to detect returning static locals in public headers

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

C++ Core Guidelines also mentioned similar patterns, so may be this check 
belongs to bugprone module?




Comment at: docs/clang-tidy/checks/llvm-problematic-statics.rst:7
+
+Looks for functions defined in "public" headers that return the address of a 
static local. These are not guaranteed to have the addresss across shared 
libraries and could be problematic for plugins.
+

static local variable? Probably //Looks for// should be replaced with 
//Detects//. Please also wrap with 80 characters.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54222



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

[PATCH] D54168: [clang-tidy] Zircon fbl::move -> std::move

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:188
+
+  Suggests converting uses of ``fbl::move`` to
+  ``std::move``, and suggests inserting the  header.

Please try to fill as much as possible to 80 characters.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:189
+  Suggests converting uses of ``fbl::move`` to
+  ``std::move``, and suggests inserting the  header.
+

inserting -> to include? Same in documentation.


https://reviews.llvm.org/D54168



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

[PATCH] D54168: [clang-tidy] Zircon fbl::move -> std::move

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/zircon/FblMoveCheck.cpp:61
+// Add in the  header.
+if (auto IncludeFixit =
+Inserter->CreateIncludeInsertion(SM.getFileID(Start), "utility",

Please don't use auto when type could not be deduced from statement.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:188
+
+  This check is part of the migration checks for moving Zircon user code to use
+  the C++ standard library. It suggests converting uses of ``fbl::move`` to

I think first statement is not necessary. Second should start from 
//Suggests//. Will be good idea to synchronize it with documentation.


https://reviews.llvm.org/D54168



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

[PATCH] D54169: [clang-tidy] Zircon ->

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:111
+  // Add in the  header, since we know this file uses it.
+  if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+  SM.getFileID(V->getLocation()), "limits",

Please don't use auto because type could not be deduced from statement.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:188
+
+  This check is part of the migration checks for moving Zircon user code to use
+  the C++ standard library. It suggests converting uses of declarations in the

I think first statement is not necessary. Second should start from 
//Suggests//. Will be good idea to synchronize it with documentation.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:190
+  the C++ standard library. It suggests converting uses of declarations in the
+   header to their std counterparts in .
+

Please highlight std with ``.


https://reviews.llvm.org/D54169



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Reducing log file size is good idea, but I think will be also good idea to 
count duplicates. This will allow to concentrate clean-up efforts on place 
where most of warnings originate.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10
+
+  .. code-block:: c++
+

JonasToth wrote:
> ztamas wrote:
> > JonasToth wrote:
> > > the `.. code-block:: c++` is usually not indended, only the code itself.
> > Hmm, I copied this from somewhere. It might be a good idea to make this 
> > consistent across all the *.rst files. See 
> > bugprone-suspicious-semicolon.rst or bugprone-use-after-move.rst for 
> > example.
> True, but nobody want's to do the documentation work :D
I could try to fix, but I need to be pointed to proper example :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974



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

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:116
+
+  This check searches for those for loops which has a loop variable with
+  a "too small" type which means this type can't represent all values

Please avoid This check, may be Detects? Same in documentation.



Comment at: docs/ReleaseNotes.rst:116
+
+  This check searches for those `for` loops which has a loop variable with
+  a "too small" type which means this type can't represent all values

Somehow for is still highlighted with single `, should be ``. Same in 
documentation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974



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

[PATCH] D53974: [clang-tidy] new checker: bugprone-too-small-loop-variable

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:26
+
+
+/// \brief The matcher for loops with suspicious integer loop variable.

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:41
+void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
+
+  const StatementMatcher LoopVarMatcher =

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:85
+
+
+/// \brief Returns the positive part of the integer width for an integer type

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:88
+unsigned calcPositiveBits(const ASTContext , const QualType 
) {
+
+  assert(IntExprType->isIntegerType());

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:96
+
+
+/// \brief Calculate the condition expression's positive bits, but ignore 
constant like values to reduce false positives

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:99
+unsigned calcCondExprPositiveBits(const ASTContext , const Expr 
*CondExpr, const QualType ) {
+
+  unsigned CondExprPosBits = 0;

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:105
+  // We are interested in variable values' positive bits
+  if(const auto *BinOperator = dyn_cast(CondExpr)) {
+const Expr *RHSE = BinOperator->getRHS()->IgnoreParenImpCasts();

Please run Clang-format.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:108
+const Expr *LHSE = BinOperator->getLHS()->IgnoreParenImpCasts();
+
+

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:114
+
+
+const QualType RHSEType = RHSE->getType();

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:121
+
+
+if(RHSEType->isEnumeralType() || RHSEType.isConstQualified() ||

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:123
+if(RHSEType->isEnumeralType() || RHSEType.isConstQualified() ||
+   RHSE->getBeginLoc().isMacroID() || isa(RHSE)) {
+

Braces are not needed.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:126
+  CondExprPosBits = calcPositiveBits(Context, LHSEType);
+
+}

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:129
+else if (LHSEType->isEnumeralType() || LHSEType.isConstQualified() ||
+ LHSE->getBeginLoc().isMacroID() || isa(LHSE)) {
+

Braces are not needed.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:130
+ LHSE->getBeginLoc().isMacroID() || isa(LHSE)) {
+
+  CondExprPosBits = calcPositiveBits(Context, RHSEType);

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:132
+  CondExprPosBits = calcPositiveBits(Context, RHSEType);
+
+} else {

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:133
+
+} else {
+

Braces are not needed.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:134
+} else {
+
+  CondExprPosBits = std::max(calcPositiveBits(Context, LHSEType),

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:138
+}
+  } else {
+

Braces are not needed.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:139
+  } else {
+
+CondExprPosBits = calcPositiveBits(Context, CondExprType);

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:141
+CondExprPosBits = calcPositiveBits(Context, CondExprType);
+
+  }

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:149
+void TooSmallLoopVariableCheck::check(const MatchFinder::MatchResult ) {
+
+  const auto *LoopVar = Result.Nodes.getNodeAs(loopVarName);

Please remove unnecessary empty line.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:153
+  const auto *LoopIncrement = 

[PATCH] D53882: [clang-tidy] Adding Zircon checker for std namespace

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:176
+
+  Warns when the `std` namespace is used, as its use is against Zircon's libc++
+  policy for the kernel.

JonasToth wrote:
> alexfh wrote:
> > JonasToth wrote:
> > > s/its/it's/
> > > 
> > > Could `std` be considered code here? Not sure, but maybe using quotes is 
> > > better?
> > Actually, "its" is correct in this context ("its use" vs "it's used").
> whupsi. sry for noise.
Please synchronize with first statement in documentation.


https://reviews.llvm.org/D53882



___
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] 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] 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] 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