[PATCH] D50389: [clang-tidy] new check for Abseil

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:8
+division of two ``absl::Duration`` objects returns an ``int64`` with any 
fractional
+component truncated toward 0. See `this link 
`_
 for more information on how ``absl::Duration`` arithmetic functions.
+

> See ... for more information on how ``absl::Duration`` arithmetic functions.

The sentence is incomplete.


https://reviews.llvm.org/D50389



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

[PATCH] D49800: [clang-tidy: modernize] modernize-redundant-void-arg crashes when a function body is in a macro

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/RedundantVoidArgCheck.cpp:246-247
+  Lambda->getLambdaClass()->getLambdaTypeInfo()->getTypeLoc();
+  End = SM->getSpellingLoc(TypeLoc.getLocEnd());
+  Begin = SM->getSpellingLoc(TypeLoc.getLocStart());
+} else {

Will this code also work in the non-macro case?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49800



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

[PATCH] D50447: [clang-tidy] Omit cases where loop variable is not used in loop body in performance-for-range-copy.

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447



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

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG


https://reviews.llvm.org/D49851



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

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

[Alexander Kornienko via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D36892#1187959, @JonasToth wrote:

> @lebedev.ri and @alexfh i would change the tests in 
> https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this 
> one?
>
> For testing purposes, you could change a single line of 
> `hicpp-exception-baseclass.cpp` to use the CHECK-NOTES. I do the rest :)


SGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D36892



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:38
+   "depends upon internal implementation details, which violates the "
+   "abseil compatibilty guidelines. These can be found at "
+   "https://abseil.io/about/compatibility";);

s/. These can be found at/; see/



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:38
+   "depends upon internal implementation details, which violates the "
+   "abseil compatibilty guidelines. These can be found at "
+   "https://abseil.io/about/compatibility";);

alexfh wrote:
> s/. These can be found at/; see/
s/abseil/Abseil/


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50542



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

[PATCH] D49800: [clang-tidy: modernize] modernize-redundant-void-arg crashes when a function body is in a macro

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG with a couple of nits. Do you need someone to commit the patch for you?




Comment at: clang-tidy/modernize/RedundantVoidArgCheck.cpp:241
+SourceLocation End, Begin;
+auto SM = Result.SourceManager;
+auto TypeLoc = Lambda->getLambdaClass()->getLambdaTypeInfo()->getTypeLoc();

s/auto /SourceManager */



Comment at: clang-tidy/modernize/RedundantVoidArgCheck.cpp:242
+auto SM = Result.SourceManager;
+auto TypeLoc = Lambda->getLambdaClass()->getLambdaTypeInfo()->getTypeLoc();
+End = SM->getSpellingLoc(TypeLoc.getLocEnd());

Use the actual type name here as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49800



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

[PATCH] D49800: [clang-tidy: modernize] modernize-redundant-void-arg crashes when a function body is in a macro

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339433: [clang-tidy: modernize] modernize-redundant-void-arg 
crashes when a function… (authored by alexfh, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49800?vs=160039&id=160103#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49800

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -445,3 +445,46 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} in function definition
   // CHECK-FIXES: DefinitionWithNoBody() = delete;
 };
+
+
+
+#define BODY {}
+#define LAMBDA1 [](void){}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+// CHECK-FIXES: LAMBDA1 [](){}
+
+#define LAMBDA2 [](void)BODY
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+// CHECK-FIXES: LAMBDA2 []()BODY
+
+#define LAMBDA3(captures, args, body) captures args body
+#define WRAP(...) __VA_ARGS__
+
+#define LAMBDA4 (void)LAMBDA3([],(void),BODY)
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+// CHECK-FIXES: LAMBDA4 (void)LAMBDA3([],(),BODY)
+
+#define LAMBDA5 []() -> void (*)(void) {return BODY;}
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+// CHECK-FIXES: LAMBDA5 []() -> void (*)() {return BODY;}
+void lambda_expression_with_macro_test(){
+  (void)LAMBDA1;
+  (void)LAMBDA2;
+  (void)LAMBDA3([], (void), BODY);
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+  // CHECK-FIXES: (void)LAMBDA3([], (), BODY);
+
+  LAMBDA4;
+  LAMBDA5;
+  WRAP((void)WRAP(WRAP(LAMBDA3(WRAP([]), WRAP((void)), WRAP(BODY);
+  // CHECK-MESSAGES: :[[@LINE-1]]:48: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+  // CHECK-FIXES: WRAP((void)WRAP(WRAP(LAMBDA3(WRAP([]), WRAP(()), 
WRAP(BODY);
+
+  (void)WRAP([](void) {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+  // CHECK-FIXES: (void)WRAP([]() {});
+
+  [](void) BODY;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
+  // CHECK-FIXES: []() BODY;
+}
Index: clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -149,6 +149,8 @@
   ProtoToken.getRawIdentifier() == "void") {
 State = SawVoid;
 VoidToken = ProtoToken;
+  } else if (ProtoToken.is(tok::TokenKind::l_paren)) {
+State = SawLeftParen;
   } else {
 State = NothingYet;
   }
@@ -235,10 +237,11 @@
 const MatchFinder::MatchResult &Result, const LambdaExpr *Lambda) {
   if (Lambda->getLambdaClass()->getLambdaCallOperator()->getNumParams() == 0 &&
   Lambda->hasExplicitParameters()) {
-SourceLocation Begin =
-Lambda->getIntroducerRange().getEnd().getLocWithOffset(1);
-SourceLocation End = Lambda->getBody()->getBeginLoc().getLocWithOffset(-1);
-removeVoidArgumentTokens(Result, SourceRange(Begin, End),
+SourceManager *SM = Result.SourceManager;
+TypeLoc TL = Lambda->getLambdaClass()->getLambdaTypeInfo()->getTypeLoc();
+removeVoidArgumentTokens(Result,
+ {SM->getSpellingLoc(TL.getBeginLoc()),
+  SM->getSpellingLoc(TL.getEndLoc())},
  "lambda expression");
   }
 }


Index: clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -445,3 +445,46 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} in function definition
   // CHECK-FIXES: DefinitionWithNoBody() = delete;
 };
+
+
+
+#define BODY {}
+#define LAMBDA1 [](void){}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: redundant void argument list in lambda expression [modernize-redundant-void-arg]

[PATCH] D49800: [clang-tidy: modernize] modernize-redundant-void-arg crashes when a function body is in a macro

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I've fixed the comments and committed the patch myself. Hope that's fine by you.


Repository:
  rL LLVM

https://reviews.llvm.org/D49800



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

[PATCH] D46633: [analyzer] add range check for InitList lookup

[Alexander Kornienko via Phabricator via cfe-commits]

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

Thank you for the fix!
LG

The fix looks trivial and I'll commit your patch to unblock our internal 
release. If there are comments from other reviewers, they can be resolved 
post-commit.


Repository:
  rC Clang

https://reviews.llvm.org/D46633



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

[PATCH] D46633: [analyzer] add range check for InitList lookup

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL331870: Fixes issue introduced by r331556. (authored by 
alexfh, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46633?vs=145880&id=145899#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46633

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
  cfe/trunk/test/Analysis/initialization.c


Index: cfe/trunk/test/Analysis/initialization.c
===
--- cfe/trunk/test/Analysis/initialization.c
+++ cfe/trunk/test/Analysis/initialization.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+void initbug() {
+  const union { float a; } u = {};
+  (void)u.a; // no-crash
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1711,13 +1711,15 @@
   if (const auto *VR = dyn_cast(superR)) {
 const VarDecl *VD = VR->getDecl();
 QualType RecordVarTy = VD->getType();
+unsigned Index = FD->getFieldIndex();
 // Either the record variable or the field has to be const qualified.
 if (RecordVarTy.isConstQualified() || Ty.isConstQualified())
   if (const Expr *Init = VD->getInit())
 if (const auto *InitList = dyn_cast(Init))
-  if (const Expr *FieldInit = InitList->getInit(FD->getFieldIndex()))
-if (Optional V = svalBuilder.getConstantVal(FieldInit))
-  return *V;
+  if (Index < InitList->getNumInits())
+if (const Expr *FieldInit = InitList->getInit(Index))
+  if (Optional V = svalBuilder.getConstantVal(FieldInit))
+return *V;
   }
 
   return getBindingForFieldOrElementCommon(B, R, Ty);


Index: cfe/trunk/test/Analysis/initialization.c
===
--- cfe/trunk/test/Analysis/initialization.c
+++ cfe/trunk/test/Analysis/initialization.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+void initbug() {
+  const union { float a; } u = {};
+  (void)u.a; // no-crash
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1711,13 +1711,15 @@
   if (const auto *VR = dyn_cast(superR)) {
 const VarDecl *VD = VR->getDecl();
 QualType RecordVarTy = VD->getType();
+unsigned Index = FD->getFieldIndex();
 // Either the record variable or the field has to be const qualified.
 if (RecordVarTy.isConstQualified() || Ty.isConstQualified())
   if (const Expr *Init = VD->getInit())
 if (const auto *InitList = dyn_cast(Init))
-  if (const Expr *FieldInit = InitList->getInit(FD->getFieldIndex()))
-if (Optional V = svalBuilder.getConstantVal(FieldInit))
-  return *V;
+  if (Index < InitList->getNumInits())
+if (const Expr *FieldInit = InitList->getInit(Index))
+  if (Optional V = svalBuilder.getConstantVal(FieldInit))
+return *V;
   }
 
   return getBindingForFieldOrElementCommon(B, R, Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D46602#1092111, @rja wrote:

> +1 for JSON


Could you explain why would you use YAML or JSON for this? How are you going to 
use/process this data?


Repository:
  rL LLVM

https://reviews.llvm.org/D46602



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Roman, it looks to me that a simpler storage scheme would be sufficient. For 
example, MMDDhhmmss-InputFileName.cpp.csv. Main things are: 1. include a 
timestamp, so there's no need to overwrite old results, 2. include just the 
name of the file without any parent directories, 3. put all outputs into the 
same directory. This way we wouldn't have to create a directory structure and 
think about stripping a certain prefix (btw, utilities like patch just specify 
the number of path components to remove from the start, not the actual 
substring). WDYT?


Repository:
  rL LLVM

https://reviews.llvm.org/D46602



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

[PATCH] D46603: [Support] TimerGroup changes

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I wonder why JSON? What kind of a tool do folks use to process the outputs of 
printJSONValue? Is there anything existing or is JSON used "just in case"? I 
personally use either spreadsheets, python or shell when I deal with this kind 
of data, and all three of them can work reasonably well with CSV (plus the data 
is pretty simple).


Repository:
  rL LLVM

https://reviews.llvm.org/D46603



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

[PATCH] D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize-use-auto-min-type-name-length.cpp:61-83
+long int li = static_cast(foo());
+// CHECK-FIXES-0-0: auto li = {{.*}}
+// CHECK-FIXES-0-5: auto li = {{.*}}
+// CHECK-FIXES-1-0: auto  li = {{.*}}
+// CHECK-FIXES-1-5: auto  li = {{.*}}
+long int *pli = static_cast(foo());
+// CHECK-FIXES-0-0: auto *pli = {{.*}}

zinovy.nis wrote:
> zinovy.nis wrote:
> > zinovy.nis wrote:
> > > alexfh wrote:
> > > > These tests could be more useful, if they verified boundary values of 
> > > > the MinTypeNameLength, e.g. that `long int` is replaced with `auto` 
> > > > when the option is set to 8 and it stays `long int`with the option set 
> > > > to 9.
> > > > 
> > > > Please also add tests with template typenames: e.g. the lenght of `T <  
> > > > int  >` should be considered 6 and the length of `T  <  const int >` is 
> > > > 12. I guess, the algorithm I proposed will be incorrect for pointer 
> > > > type arguments of templates (the length of `T` should be 7 
> > > > regardless of `RemoveStars`), but this can be fixed by conditionally 
> > > > (on RemoveStars) trimming `*`s from the right of the type name and then 
> > > > treating them as punctuation. So instead of
> > > > 
> > > > ```
> > > >   for (const unsigned char C : tooling::fixit::getText(SR, Context)) {
> > > > const CharType NextChar =
> > > > std::isalnum(C)
> > > > ? Alpha
> > > > : (std::isspace(C) || (!RemoveStars && C == '*')) ? Space
> > > >   : 
> > > > Punctuation;
> > > > ```
> > > > 
> > > > you could use something similar to
> > > > 
> > > > ```
> > > >   StringRef Text = tooling::fixit::getText(SR, Context);
> > > >   if (RemoveStars)
> > > > Text = Text.rtrim(" \t\v\n\r*");
> > > >   for (const unsigned char C : Text) {
> > > > const CharType NextChar =
> > > > std::isalnum(C) ? Alpha : std::isspace(C) ? Space : Punctuation;
> > > > ```
> > > > These tests could be more useful, if they verified boundary values of 
> > > > the MinTypeNameLength, e.g. that long int is replaced with auto when 
> > > > the option is set to 8 and it stays long intwith the option set to 9.
> > > > 
> > > 
> > > `int`-test is just for that :-)
> > > 
> > > ```
> > > int a = static_cast(foo()); // ---> int  a = ...
> > > ```
> > I measured lengths for template cases:
> > 
> > 
> > ```
> > S=std::string *   len=12
> > S=std::vector *   len=25
> > S=std::vector *   len=31
> > S=std::string*   len=12
> > S=std::vector   *   len=25
> > S=std::vector*   len=31
> > ```
> > 
> > 
> RemoveStars==1 here.
> S=std::string *   len=12

With RemoveStars this should be 11?

> S=std::vector *   len=25

24?

> S=std::vector *   len=31

30?

The point of my comment was that stars should be treated specially only at the 
end of the type, not inside template parameters, e.g. `T` should have 
length 10 regardless of RemoveStars. All your examples are with trailing stars.



Comment at: clang-tidy/modernize-use-auto-min-type-name-length.cpp:61-83
+long int li = static_cast(foo());
+// CHECK-FIXES-0-0: auto li = {{.*}}
+// CHECK-FIXES-0-5: auto li = {{.*}}
+// CHECK-FIXES-1-0: auto  li = {{.*}}
+// CHECK-FIXES-1-5: auto  li = {{.*}}
+long int *pli = static_cast(foo());
+// CHECK-FIXES-0-0: auto *pli = {{.*}}

alexfh wrote:
> zinovy.nis wrote:
> > zinovy.nis wrote:
> > > zinovy.nis wrote:
> > > > alexfh wrote:
> > > > > These tests could be more useful, if they verified boundary values of 
> > > > > the MinTypeNameLength, e.g. that `long int` is replaced with `auto` 
> > > > > when the option is set to 8 and it stays `long int`with the option 
> > > > > set to 9.
> > > > > 
> > > > > Please also add tests with template typenames: e.g. the lenght of `T 
> > > > > <  int  >` should be considered 6 and the length of `T  <  const int 
> > > > > >` is 12. I guess, the algorithm I proposed will be incorrect for 
> > > > > pointer type arguments of templates (the length of `T` should 
> > > > > be 7 regardless of `RemoveStars`), but this can be fixed by 
> > > > > conditionally (on RemoveStars) trimming `*`s from the right of the 
> > > > > type name and then treating them as punctuation. So instead of
> > > > > 
> > > > > ```
> > > > >   for (const unsigned char C : tooling::fixit::getText(SR, Context)) {
> > > > > const CharType NextChar =
> > > > > std::isalnum(C)
> > > > > ? Alpha
> > > > > : (std::isspace(C) || (!RemoveStars && C == '*')) ? Space
> > > > >   : 
> > > > > Punctuation;
> > > > > ```
> > > > > 
> > > > > you could use something similar to
> > > > > 
> > > > > ```
> > > > >   StringRef Text = tooling::fixit::getText(SR, Context);
> > > > >   if (RemoveStars)
> > > > >

[PATCH] D33844: [clang-tidy] terminating continue check

[Alexander Kornienko via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D33844#1095862, @koldaniel wrote:

> In https://reviews.llvm.org/D33844#1086565, @alexfh wrote:
>
> >
>
>
> Which comments do you think are still relevant?


Weird. When I was writing the comment, the code looked identical to the version 
from Mar 27. It may have been something wrong with my browser, with the 
Phabricator, or with the way I used it. Anyway, looks good now. Sorry for the 
confusion.

Do you need someone to commit the patch for you?


https://reviews.llvm.org/D33844



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote:

> In https://reviews.llvm.org/D46602#1092890, @alexfh wrote:
>
> > Roman, it looks to me that a simpler storage scheme would be sufficient. 
> > For example, MMDDhhmmss-InputFileName.cpp.csv.
> >  Main things are:
> >
> > 1. include a timestamp, so there's no need to overwrite old results,
>
>
> Of the input source file?


No, current timestamp, when each profile gets dumped.

>> 2. include just the name of the file without any parent directories,
> 
> That won't work, there are duplicate filenames even in LLVM.

That's why I suggested to include a timestamp. Each result file will get a 
unique timestamp as a part of its name (unless the resolution of the timestamp 
is too coarse - compared to clang-tidy run time - to allow collisions).

> $ find -iname Path.cpp
>  ./lib/Support/Path.cpp
>  ./unittests/Support/Path.cpp
> 
>   > 3. put all outputs into the same directory. This way we wouldn't have to 
> create a directory structure and think about stripping a certain prefix (btw, 
> utilities like patch just specify the number of path components to remove 
> from the start, not the actual substring). WDYT?
>
>I'm not particularly looking forward to having being forced to have n 
> thousands of reports in a single directory :)

What kind of problems do you expect to have with this? Are you going to look at 
the result files manually or use a script to aggregate them?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/D46602#1095980, @lebedev.ri wrote:

> In https://reviews.llvm.org/D46602#1095960, @alexfh wrote:
>
> > In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote:
> >
> > > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote:
> > >
> > > > Roman, it looks to me that a simpler storage scheme would be 
> > > > sufficient. For example, MMDDhhmmss-InputFileName.cpp.csv.
> > > >  Main things are:
> > > >
> > > > 1. include a timestamp, so there's no need to overwrite old results,
> > >
> > >
> > > Of the input source file?
> >
> >
> > No, current timestamp, when each profile gets dumped.
>
>
> Ah.
>  Then i don't understand this at all. Why would we want to do that, exactly?
>  Just so that we can avoid creating directory structure? Why do we want to 
> avoid that?


If clang-tidy is invoked manually, a simpler naming scheme with less 
configuration options would be easier to use, in particular:

1. no the need to specify the `-store-check-profile-elide-prefix=` option;
2. it's easier to see all results (no need to use `find`, just `ls 
/output/directory` or `ls /output/directory/20180514*` to see today's results, 
for example);
3. no chance of filename collision, and thus no chance of losing older results 
by just running clang-tidy again.

If clang-tidy is started by a script (e.g. run-clang-tidy.py or a modification 
of it specialized on profiling), then a specific naming scheme doesn't matter 
much, since the script can create a separate directory for each run or for each 
shard. I don't yet see how repeating the directory structure of the input files 
would make profiling results easier or more convenient to use. A relation 
between the input file and the result file can still be maintained in some 
other form (for example, by specifying the whole invocation inside the results 
file - if we want to use YAML).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602



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

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D46159#1098000, @pfultz2 wrote:

> Is someone able to merge in my changes?


Will do.


https://reviews.llvm.org/D46159



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

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D46159#1098018, @alexfh wrote:

> In https://reviews.llvm.org/D46159#1098000, @pfultz2 wrote:
>
> > Is someone able to merge in my changes?
>
>
> Will do.


Please rebase the patch, it doesn't apply cleanly.


https://reviews.llvm.org/D46159



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D46602#1097954, @lebedev.ri wrote:

> In https://reviews.llvm.org/D46602#1097902, @alexfh wrote:
>
> > In https://reviews.llvm.org/D46602#1095980, @lebedev.ri wrote:
> >
> > > In https://reviews.llvm.org/D46602#1095960, @alexfh wrote:
> > >
> > > > In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote:
> > > >
> > > > > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote:
> > > > >
> > > > > > Roman, it looks to me that a simpler storage scheme would be 
> > > > > > sufficient. For example, MMDDhhmmss-InputFileName.cpp.csv.
> > > > > >  Main things are:
> > > > > >
> > > > > > 1. include a timestamp, so there's no need to overwrite old results,
> > > > >
> > > > >
> > > > > Of the input source file?
> > > >
> > > >
> > > > No, current timestamp, when each profile gets dumped.
> > >
> > >
> > > Ah.
> > >  Then i don't understand this at all. Why would we want to do that, 
> > > exactly?
> > >  Just so that we can avoid creating directory structure? Why do we want 
> > > to avoid that?
> >
> >
> > If clang-tidy is invoked manually, a simpler naming scheme with less 
> > configuration options would be easier to use, in particular:
> >
> > 1. no the need to specify the `-store-check-profile-elide-prefix=` option;
> > 2. it's easier to see all results (no need to use `find`, just `ls 
> > /output/directory` or `ls /output/directory/20180514*` to see today's 
> > results, for example);
> > 3. no chance of filename collision, and thus no chance of losing older 
> > results by just running clang-tidy again.
>
>
> How do i reflect that in tests? The output name will basically be random.


Why random? It will follow a certain rule that can be verified. If we decide to 
name the file `MMDD_InputFileName.cpp.yaml`, for example, then the test can 
verify the file matching `_InputFileName.cpp.yaml` exists and contains 
whatever it should contain. If we want the test to be stricter, it could verify 
that the `` part matches a certain pattern (say, 
`20\d\d[0-1][0-9][0-3][0-9]`). Additionally, it could verify that after a short 
sleep a file with a different `` part is generated.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602



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

[PATCH] D46951: [clang-tidy] misc-unused-parameters - retain old behavior under StrictMode

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh created this revision.
alexfh added reviewers: klimek, ilya-biryukov.
Herald added a subscriber: xazax.hun.

This addresses https://bugs.llvm.org/show_bug.cgi?id=37467.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46951

Files:
  clang-tidy/misc/UnusedParametersCheck.cpp
  clang-tidy/misc/UnusedParametersCheck.h
  docs/clang-tidy/checks/misc-unused-parameters.rst
  test/clang-tidy/misc-unused-parameters-strict.cpp
  test/clang-tidy/misc-unused-parameters.cpp

Index: test/clang-tidy/misc-unused-parameters.cpp
===
--- test/clang-tidy/misc-unused-parameters.cpp
+++ test/clang-tidy/misc-unused-parameters.cpp
@@ -222,5 +222,34 @@
   return Function();
 }
 
+namespace strict_mode_off {
 // Do not warn on empty function bodies.
 void f(int foo) {}
+class E {
+  int i;
+
+public:
+  E(int j) {}
+};
+class F {
+  int i;
+
+public:
+  // Constructor initializer counts as a non-empty body.
+  F(int j) : i() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'j' is unused
+// CHECK-FIXES: {{^}}  F(int  /*j*/) : i() {}{{$}}
+};
+
+class A {
+public:
+  A();
+  A(int);
+};
+class B : public A {
+public:
+  B(int i) : A() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'i' is unused
+// CHECK-FIXES: {{^}}  B(int  /*i*/) : A() {}{{$}}
+};
+} // namespace strict_mode_off
Index: test/clang-tidy/misc-unused-parameters-strict.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-unused-parameters-strict.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s misc-unused-parameters %t -- \
+// RUN:   -config="{CheckOptions: [{key: StrictMode, value: 1}]}" --
+
+// Warn on empty function bodies in StrictMode.
+namespace strict_mode {
+void f(int foo) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: parameter 'foo' is unused [misc-unused-parameters]
+// CHECK-FIXES: {{^}}void f(int  /*foo*/) {}{{$}}
+class E {
+  int i;
+
+public:
+  E(int j) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'j' is unused
+// CHECK-FIXES: {{^}}  E(int  /*j*/) {}{{$}}
+};
+class F {
+  int i;
+
+public:
+  F(int j) : i() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'j' is unused
+// CHECK-FIXES: {{^}}  F(int  /*j*/) : i() {}{{$}}
+};
+}
Index: docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- docs/clang-tidy/checks/misc-unused-parameters.rst
+++ docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -3,24 +3,40 @@
 misc-unused-parameters
 ==
 
-Finds unused parameters and fixes them, so that `-Wunused-parameter` can be
-turned on.
+Finds unused function parameters. Unused parameters may signal about a bug in
+the code (e.g. when a different parameter is used instead). The suggested fixes
+either comment parameter name out or remove the parameter completely, if all
+callers of the function are in the same translation unit and can be updated.
+
+The check is similar to the `-Wunused-parameter` compiler diagnostic and can be
+used to prepare a codebase to enabling of that diagnostic. By default the check
+is more permissive (see :option:`StrictMode`).
 
 .. code-block:: c++
 
-  void a(int i) {}
+  void a(int i) { /*some code that doesn't use `i`*/ }
 
   // becomes
 
-  void a(int  /*i*/) {}
-
+  void a(int  /*i*/) { /*some code that doesn't use `i`*/ }
 
 .. code-block:: c++
 
   static void staticFunctionA(int i);
-  static void staticFunctionA(int i) {}
+  static void staticFunctionA(int i) { /*some code that doesn't use `i`*/ }
 
   // becomes
 
   static void staticFunctionA()
-  static void staticFunctionA() {}
+  static void staticFunctionA() { /*some code that doesn't use `i`*/ }
+
+Options
+---
+
+.. option:: StrictMode
+
+   When zero (default value), the check will ignore trivially unused parameters,
+   i.e. when the corresponding function has an empty body (and in case of
+   constructors - no constructor initializers). When the function body is empty,
+   an unused parameter is unlikely to be unnoticed by a human reader, and
+   there's basically no place for a bug to hide in.
Index: clang-tidy/misc/UnusedParametersCheck.h
===
--- clang-tidy/misc/UnusedParametersCheck.h
+++ clang-tidy/misc/UnusedParametersCheck.h
@@ -24,8 +24,10 @@
   ~UnusedParametersCheck();
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
+  const bool StrictMode;
   class IndexerVisitor;
   std::unique_ptr Indexer;
 
Index: clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tidy/misc/UnusedParametersCheck.cpp
@@ -29,11 +29,10 @@
 } // namespace

[PATCH] D33440: clang-format: better handle statement macros

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

There are still outstanding comments.


Repository:
  rC Clang

https://reviews.llvm.org/D33440



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

[PATCH] D46939: [Timers] TimerGroup: add constructor from StringMap

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG


Repository:
  rL LLVM

https://reviews.llvm.org/D46939



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

[PATCH] D46938: [Timers] TimerGroup: make printJSONValues() method public

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG


Repository:
  rL LLVM

https://reviews.llvm.org/D46938



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

[PATCH] D46936: [Timers] TimerGroup::printJSONValues(): print mem timer with .mem suffix

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG


Repository:
  rL LLVM

https://reviews.llvm.org/D46936



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

[PATCH] D46659: [clang-tidy/google-readability-casting] Disable check for Objective-C++

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.

LG


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46659



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

A couple of comments from a cursory look. I'll try to look closer later this 
week.




Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:173
+  /// \brief Control storage of profile date.
+  void setStoreProfile(StringRef ProfilePrefix);
+  llvm::Optional getStoreProfile() const;

The name is misleading. I'd suggest using a name that unambiguously states that 
we're setting a prefix for profiling output files. Some ideas: 
setProfilePathPrefix, setStoredProfilePathPrefix, setProfileStorePathPrefix, 
setProfileResultsPathPrefix.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:174
+  void setStoreProfile(StringRef ProfilePrefix);
+  llvm::Optional getStoreProfile() const;
+

getProfileStorageParams?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602



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

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

This patch seems to cause an assertion failure:

assert.h assertion failed at clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:427 
in clang::ento::SVal 
clang::ento::SValBuilder::evalBinOp(clang::ento::ProgramStateRef, 
BinaryOperator::Opcode, clang::ento::SVal, clang::ento::SVal, clang::QualType): 
op == BO_Add

The stack trace is:
__assert_fail
clang::ento::SValBuilder::evalBinOp
clang::ento::SValBuilder::evalEQ
clang::ento::SValBuilder::evalEQ
::CStringChecker::assumeZero
::CStringChecker::checkNonNull
::CStringChecker::evalStrcpyCommon
::CStringChecker::evalStrcpy
::CStringChecker::evalCall
clang::ento::eval::Call::_evalCall
clang::ento::CheckerFn::operator()
clang::ento::CheckerManager::runCheckersForEvalCall
clang::ento::ExprEngine::evalCall
clang::ento::ExprEngine::VisitCallExpr
clang::ento::ExprEngine::Visit
clang::ento::ExprEngine::ProcessStmt
clang::ento::ExprEngine::processCFGElement
clang::ento::CoreEngine::HandlePostStmt
clang::ento::CoreEngine::dispatchWorkItem
clang::ento::CoreEngine::ExecuteWorkList
clang::ento::ExprEngine::ExecuteWorkList
::AnalysisConsumer::ActionExprEngine
::AnalysisConsumer::HandleCode
::AnalysisConsumer::HandleDeclsCallGraph
::AnalysisConsumer::runAnalysisOnTranslationUnit
::AnalysisConsumer::HandleTranslationUnit

I'm trying to reduce a test case.


Repository:
  rC Clang

https://reviews.llvm.org/D45177



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

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

This is reproducible in r332425.


Repository:
  rC Clang

https://reviews.llvm.org/D45177



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

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332609: [clang-tidy] Add a flag to enable alpha checkers 
(authored by alexfh, committed by ).
Herald added subscribers: llvm-commits, klimek.

Changed prior to commit:
  https://reviews.llvm.org/D46159?vs=146719&id=147311#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46159

Files:
  clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
  clang-tools-extra/trunk/clang-tidy/ClangTidy.h
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/trunk/test/clang-tidy/enable-alpha-checks.cpp

Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -177,9 +177,11 @@
 };
 
 ClangTidyContext::ClangTidyContext(
-std::unique_ptr OptionsProvider)
+std::unique_ptr OptionsProvider,
+bool AllowEnablingAnalyzerAlphaCheckers)
 : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
-  Profile(false) {
+  Profile(false),
+  AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
   // Before the first translation unit we can get errors related to command-line
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
Index: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
@@ -309,11 +309,12 @@
 
 typedef std::vector> CheckersList;
 
-static CheckersList getCheckersControlList(ClangTidyContext &Context) {
+static CheckersList getCheckersControlList(ClangTidyContext &Context,
+   bool IncludeExperimental) {
   CheckersList List;
 
   const auto &RegisteredCheckers =
-  AnalyzerOptions::getRegisteredCheckers(/*IncludeExperimental=*/false);
+  AnalyzerOptions::getRegisteredCheckers(IncludeExperimental);
   bool AnalyzerChecksEnabled = false;
   for (StringRef CheckName : RegisteredCheckers) {
 std::string ClangTidyCheckName((AnalyzerCheckNamePrefix + CheckName).str());
@@ -379,7 +380,8 @@
 Consumers.push_back(Finder->newASTConsumer());
 
   AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts();
-  AnalyzerOptions->CheckersControlList = getCheckersControlList(Context);
+  AnalyzerOptions->CheckersControlList =
+  getCheckersControlList(Context, Context.canEnableAnalyzerAlphaCheckers());
   if (!AnalyzerOptions->CheckersControlList.empty()) {
 setStaticAnalyzerCheckerOpts(Context.getOptions(), AnalyzerOptions);
 AnalyzerOptions->AnalysisStoreOpt = RegionStoreModel;
@@ -404,7 +406,8 @@
   CheckNames.push_back(CheckFactory.first);
   }
 
-  for (const auto &AnalyzerCheck : getCheckersControlList(Context))
+  for (const auto &AnalyzerCheck : getCheckersControlList(
+   Context, Context.canEnableAnalyzerAlphaCheckers()))
 CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 
   std::sort(CheckNames.begin(), CheckNames.end());
@@ -463,18 +466,24 @@
   store(Options, LocalName, llvm::itostr(Value));
 }
 
-std::vector getCheckNames(const ClangTidyOptions &Options) {
+std::vector
+getCheckNames(const ClangTidyOptions &Options,
+  bool AllowEnablingAnalyzerAlphaCheckers) {
   clang::tidy::ClangTidyContext Context(
   llvm::make_unique(ClangTidyGlobalOptions(),
-Options));
+Options),
+  AllowEnablingAnalyzerAlphaCheckers);
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckNames();
 }
 
-ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options) {
+ClangTidyOptions::OptionMap
+getCheckOptions(const ClangTidyOptions &Options,
+bool AllowEnablingAnalyzerAlphaCheckers) {
   clang::tidy::ClangTidyContext Context(
   llvm::make_unique(ClangTidyGlobalOptions(),
-Options));
+Options),
+  AllowEnablingAnalyzerAlphaCheckers);
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -99,7 +99,8 @@
 class ClangTidyContext {
 public:
   /// \brief Initializes \c ClangTidyContext instance.
-  ClangTidyContext(std::unique_ptr OptionsP

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.


Repository:
  rC Clang

https://reviews.llvm.org/D45177



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

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D45177#1103774, @devnexen wrote:

> In https://reviews.llvm.org/D45177#1103162, @alexfh wrote:
>
> > See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.
>
>
> I was unable to reproduce both FreeBSD and Linux. Plus my changes come after 
> checkNonNull.


I'm not 100% sure this was caused by your patch, but the stack trace looks 
suspiciously similar to what was changed here. As for not being able to 
reproduce: do you build Clang with assertions enabled?


Repository:
  rC Clang

https://reviews.llvm.org/D45177



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

[PATCH] D38171: Implement clang-tidy check aliases.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote:

> On a somewhat related  note, since this is already talking about aliases
>
> I feel like the current handling of the clang-tidy check aliases needs 
> adjusting.
>  If one enables all the checks (`Checks: '*'`), and then disables some of them
>  on check-by-check basis,


Clang-tidy allows doing so, but I doubt this is a use case we want to 
encourage. At least, not for a day-to-day use. Most checks require a certain 
amount of work on the target code and sometimes fine tuning of the check 
options to be useful for any real codebase. Apart from that, there are checks 
that give duplicate warnings (some are aliases and some just cover intersecting 
set of issues), and there are checks that can give conflicting advice. These 
facts don't combine well with the "enable almost all checks" use case and limit 
its area of applicability to stress-testing clang-tidy and maybe to initial 
evaluation of clang-tidy checks on a certain codebase (though going from the 
other direction and determining the set of style rules for the codebase and 
then finding which of those are implemented in clang-tidy is a more fruitful 
approach).

> if the disabled check has aliases
>  (`cppcoreguidelines-pro-type-vararg` vs `hicpp-vararg`, 
> `hicpp-no-array-decay`
>  vs `cppcoreguidelines-pro-bounds-array-to-pointer-decay` and so on)
>  each of the aliases must be explicitly be disabled too.

It seems to be the least confusing of possible options. Gabor made a good point 
why this is useful as it is.

> This is rather inconvenient.
> 
> If that is intentional, perhaps there could be a config option to either 
> disable
>  creation/registration of the check aliases altogether, or an option to threat
>  aliases as a hard link, so anything happening to any of the aliases/base 
> check
>  would happen to all of them.
> 
> (Also, if the check has parameters, and one specifies different parameters 
> for the base check, and the aliases, what happens?)




https://reviews.llvm.org/D38171



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

[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thank you, Artem!


Repository:
  rL LLVM

https://reviews.llvm.org/D37023



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

[PATCH] D38179: [clang-tidy] Handle unions in modernize-use-equals-default check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:119-120
+
+  // A defaulted default constructor of a union with a field with a non trivial
+  // default constructor would be deleted.
+  if (Record->isUnion()) {

malcolm.parsons wrote:
> aaron.ballman wrote:
> > This does not match what's in [class.ctor]p5.
> > 
> > "X is a union that has a variant member with a non-trivial default 
> > constructor and no variant member of X has a default member initializer" -- 
> > you aren't checking for the default member initializer.
> > 
> > also missing for unions: "X is a union and all of its variant members are 
> > of const-qualified type (or array thereof)"
> > 
> > (You should add test cases for these.)
> > 
> > It might make more sense to implement this as 
> > `CXXRecordDecl::defaultedDestructorIsDeleted()`?
> Is there any way to call `Sema::ShouldDeleteSpecialMember()` from a 
> clang-tidy check?
By the time clang-tidy checks are invoked, Sema is already dead, and I don't 
think there's a reasonable way to use it.



Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:127
+
+  if (const RecordType *RecordTy = T->getAs()) {
+CXXRecordDecl *FieldRec = cast(RecordTy->getDecl());

malcolm.parsons wrote:
> aaron.ballman wrote:
> > You can use `auto` here and below.
> This was copied from `CXXRecordDecl::addedMember()`.
Not all LLVM/Clang code is ideal. It makes sense to clean up issues once the 
code is changed and even more so when it's copied elsewhere.


https://reviews.llvm.org/D38179



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

[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thank you for the fix! It seems to be the largest source of SA crashes on our 
code at the moment.


https://reviews.llvm.org/D38358



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

[PATCH] D38458: Fix assertion failure in thread safety analysis (PR34800).

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh created this revision.

Fix an assertion failure and clean up unused code relevant to the fixed logic.


https://reviews.llvm.org/D38458

Files:
  include/clang/Analysis/Analyses/ThreadSafetyTIL.h
  test/SemaCXX/warn-thread-safety-analysis.cpp


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5233,3 +5233,18 @@
   } // expected-warning {{mutex 'lock_' is still held at the end of function}}
   Mutex lock_ ACQUIRED_BEFORE("");
 };
+
+namespace PR34800 {
+struct A {
+  operator int() const;
+};
+struct B {
+  bool g() __attribute__((locks_excluded(h))); // expected-warning 
{{'locks_excluded' attribute requires arguments whose type is annotated with 
'capability' attribute; type here is 'int'}}
+  int h;
+};
+struct C {
+  B *operator[](int);
+};
+C c;
+void f() { c[A()]->g(); }
+} // namespace PR34800
Index: include/clang/Analysis/Analyses/ThreadSafetyTIL.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -909,15 +909,10 @@
 public:
   static bool classof(const SExpr *E) { return E->opcode() == COP_Project; }
 
-  Project(SExpr *R, StringRef SName)
-  : SExpr(COP_Project), Rec(R), SlotName(SName), Cvdecl(nullptr)
-  { }
   Project(SExpr *R, const clang::ValueDecl *Cvd)
-  : SExpr(COP_Project), Rec(R), SlotName(Cvd->getName()), Cvdecl(Cvd)
-  { }
-  Project(const Project &P, SExpr *R)
-  : SExpr(P), Rec(R), SlotName(P.SlotName), Cvdecl(P.Cvdecl)
-  { }
+  : SExpr(COP_Project), Rec(R), Cvdecl(Cvd) {
+assert(Cvd && "ValueDecl must not be null");
+  }
 
   SExpr *record() { return Rec; }
   const SExpr *record() const { return Rec; }
@@ -931,10 +926,15 @@
   }
 
   StringRef slotName() const {
-if (Cvdecl)
+if (Cvdecl->getDeclName().isIdentifier())
   return Cvdecl->getName();
-else
-  return SlotName;
+if (!SlotName) {
+  std::string Buffer;
+  llvm::raw_string_ostream OS(Buffer);
+  Cvdecl->printName(OS);
+  SlotName = OS.str();
+}
+return *SlotName;
   }
 
   template 
@@ -953,7 +953,7 @@
 
 private:
   SExpr* Rec;
-  StringRef SlotName;
+  mutable llvm::Optional SlotName;
   const clang::ValueDecl *Cvdecl;
 };
 


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5233,3 +5233,18 @@
   } // expected-warning {{mutex 'lock_' is still held at the end of function}}
   Mutex lock_ ACQUIRED_BEFORE("");
 };
+
+namespace PR34800 {
+struct A {
+  operator int() const;
+};
+struct B {
+  bool g() __attribute__((locks_excluded(h))); // expected-warning {{'locks_excluded' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'int'}}
+  int h;
+};
+struct C {
+  B *operator[](int);
+};
+C c;
+void f() { c[A()]->g(); }
+} // namespace PR34800
Index: include/clang/Analysis/Analyses/ThreadSafetyTIL.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -909,15 +909,10 @@
 public:
   static bool classof(const SExpr *E) { return E->opcode() == COP_Project; }
 
-  Project(SExpr *R, StringRef SName)
-  : SExpr(COP_Project), Rec(R), SlotName(SName), Cvdecl(nullptr)
-  { }
   Project(SExpr *R, const clang::ValueDecl *Cvd)
-  : SExpr(COP_Project), Rec(R), SlotName(Cvd->getName()), Cvdecl(Cvd)
-  { }
-  Project(const Project &P, SExpr *R)
-  : SExpr(P), Rec(R), SlotName(P.SlotName), Cvdecl(P.Cvdecl)
-  { }
+  : SExpr(COP_Project), Rec(R), Cvdecl(Cvd) {
+assert(Cvd && "ValueDecl must not be null");
+  }
 
   SExpr *record() { return Rec; }
   const SExpr *record() const { return Rec; }
@@ -931,10 +926,15 @@
   }
 
   StringRef slotName() const {
-if (Cvdecl)
+if (Cvdecl->getDeclName().isIdentifier())
   return Cvdecl->getName();
-else
-  return SlotName;
+if (!SlotName) {
+  std::string Buffer;
+  llvm::raw_string_ostream OS(Buffer);
+  Cvdecl->printName(OS);
+  SlotName = OS.str();
+}
+return *SlotName;
   }
 
   template 
@@ -953,7 +953,7 @@
 
 private:
   SExpr* Rec;
-  StringRef SlotName;
+  mutable llvm::Optional SlotName;
   const clang::ValueDecl *Cvdecl;
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38458: Fix assertion failure in thread safety analysis (PR34800).

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: include/clang/Analysis/Analyses/ThreadSafetyTIL.h:931-936
+if (!SlotName) {
+  std::string Buffer;
+  llvm::raw_string_ostream OS(Buffer);
+  Cvdecl->printName(OS);
+  SlotName = OS.str();
+}

BTW, alternatively, I could do:
```
 if (!SlotName) {
   SlotName = "";
   llvm::raw_string_ostream OS(*SlotName);
   Cvdecl->printName(OS);
 }
```

Not sure which of these would be preferred in this code.


https://reviews.llvm.org/D38458



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

[PATCH] D38458: Fix assertion failure in thread safety analysis (PR34800).

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh updated this revision to Diff 117650.
alexfh added a comment.

Get rid of a local string variable.


https://reviews.llvm.org/D38458

Files:
  include/clang/Analysis/Analyses/ThreadSafetyTIL.h
  test/SemaCXX/warn-thread-safety-analysis.cpp


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5233,3 +5233,18 @@
   } // expected-warning {{mutex 'lock_' is still held at the end of function}}
   Mutex lock_ ACQUIRED_BEFORE("");
 };
+
+namespace PR34800 {
+struct A {
+  operator int() const;
+};
+struct B {
+  bool g() __attribute__((locks_excluded(h))); // expected-warning 
{{'locks_excluded' attribute requires arguments whose type is annotated with 
'capability' attribute; type here is 'int'}}
+  int h;
+};
+struct C {
+  B *operator[](int);
+};
+C c;
+void f() { c[A()]->g(); }
+} // namespace PR34800
Index: include/clang/Analysis/Analyses/ThreadSafetyTIL.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -909,15 +909,10 @@
 public:
   static bool classof(const SExpr *E) { return E->opcode() == COP_Project; }
 
-  Project(SExpr *R, StringRef SName)
-  : SExpr(COP_Project), Rec(R), SlotName(SName), Cvdecl(nullptr)
-  { }
   Project(SExpr *R, const clang::ValueDecl *Cvd)
-  : SExpr(COP_Project), Rec(R), SlotName(Cvd->getName()), Cvdecl(Cvd)
-  { }
-  Project(const Project &P, SExpr *R)
-  : SExpr(P), Rec(R), SlotName(P.SlotName), Cvdecl(P.Cvdecl)
-  { }
+  : SExpr(COP_Project), Rec(R), Cvdecl(Cvd) {
+assert(Cvd && "ValueDecl must not be null");
+  }
 
   SExpr *record() { return Rec; }
   const SExpr *record() const { return Rec; }
@@ -931,10 +926,14 @@
   }
 
   StringRef slotName() const {
-if (Cvdecl)
+if (Cvdecl->getDeclName().isIdentifier())
   return Cvdecl->getName();
-else
-  return SlotName;
+if (!SlotName) {
+  SlotName = "";
+  llvm::raw_string_ostream OS(*SlotName);
+  Cvdecl->printName(OS);
+}
+return *SlotName;
   }
 
   template 
@@ -953,7 +952,7 @@
 
 private:
   SExpr* Rec;
-  StringRef SlotName;
+  mutable llvm::Optional SlotName;
   const clang::ValueDecl *Cvdecl;
 };
 


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5233,3 +5233,18 @@
   } // expected-warning {{mutex 'lock_' is still held at the end of function}}
   Mutex lock_ ACQUIRED_BEFORE("");
 };
+
+namespace PR34800 {
+struct A {
+  operator int() const;
+};
+struct B {
+  bool g() __attribute__((locks_excluded(h))); // expected-warning {{'locks_excluded' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'int'}}
+  int h;
+};
+struct C {
+  B *operator[](int);
+};
+C c;
+void f() { c[A()]->g(); }
+} // namespace PR34800
Index: include/clang/Analysis/Analyses/ThreadSafetyTIL.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -909,15 +909,10 @@
 public:
   static bool classof(const SExpr *E) { return E->opcode() == COP_Project; }
 
-  Project(SExpr *R, StringRef SName)
-  : SExpr(COP_Project), Rec(R), SlotName(SName), Cvdecl(nullptr)
-  { }
   Project(SExpr *R, const clang::ValueDecl *Cvd)
-  : SExpr(COP_Project), Rec(R), SlotName(Cvd->getName()), Cvdecl(Cvd)
-  { }
-  Project(const Project &P, SExpr *R)
-  : SExpr(P), Rec(R), SlotName(P.SlotName), Cvdecl(P.Cvdecl)
-  { }
+  : SExpr(COP_Project), Rec(R), Cvdecl(Cvd) {
+assert(Cvd && "ValueDecl must not be null");
+  }
 
   SExpr *record() { return Rec; }
   const SExpr *record() const { return Rec; }
@@ -931,10 +926,14 @@
   }
 
   StringRef slotName() const {
-if (Cvdecl)
+if (Cvdecl->getDeclName().isIdentifier())
   return Cvdecl->getName();
-else
-  return SlotName;
+if (!SlotName) {
+  SlotName = "";
+  llvm::raw_string_ostream OS(*SlotName);
+  Cvdecl->printName(OS);
+}
+return *SlotName;
   }
 
   template 
@@ -953,7 +952,7 @@
 
 private:
   SExpr* Rec;
-  StringRef SlotName;
+  mutable llvm::Optional SlotName;
   const clang::ValueDecl *Cvdecl;
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38458: Fix assertion failure in thread safety analysis (PR34800).

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314895: Fix assertion failure in thread safety analysis 
(PR34800). (authored by alexfh).

Repository:
  rL LLVM

https://reviews.llvm.org/D38458

Files:
  cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
  cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp


Index: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
===
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -909,15 +909,10 @@
 public:
   static bool classof(const SExpr *E) { return E->opcode() == COP_Project; }
 
-  Project(SExpr *R, StringRef SName)
-  : SExpr(COP_Project), Rec(R), SlotName(SName), Cvdecl(nullptr)
-  { }
   Project(SExpr *R, const clang::ValueDecl *Cvd)
-  : SExpr(COP_Project), Rec(R), SlotName(Cvd->getName()), Cvdecl(Cvd)
-  { }
-  Project(const Project &P, SExpr *R)
-  : SExpr(P), Rec(R), SlotName(P.SlotName), Cvdecl(P.Cvdecl)
-  { }
+  : SExpr(COP_Project), Rec(R), Cvdecl(Cvd) {
+assert(Cvd && "ValueDecl must not be null");
+  }
 
   SExpr *record() { return Rec; }
   const SExpr *record() const { return Rec; }
@@ -931,10 +926,14 @@
   }
 
   StringRef slotName() const {
-if (Cvdecl)
+if (Cvdecl->getDeclName().isIdentifier())
   return Cvdecl->getName();
-else
-  return SlotName;
+if (!SlotName) {
+  SlotName = "";
+  llvm::raw_string_ostream OS(*SlotName);
+  Cvdecl->printName(OS);
+}
+return *SlotName;
   }
 
   template 
@@ -953,7 +952,7 @@
 
 private:
   SExpr* Rec;
-  StringRef SlotName;
+  mutable llvm::Optional SlotName;
   const clang::ValueDecl *Cvdecl;
 };
 
Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5233,3 +5233,18 @@
   } // expected-warning {{mutex 'lock_' is still held at the end of function}}
   Mutex lock_ ACQUIRED_BEFORE("");
 };
+
+namespace PR34800 {
+struct A {
+  operator int() const;
+};
+struct B {
+  bool g() __attribute__((locks_excluded(h))); // expected-warning 
{{'locks_excluded' attribute requires arguments whose type is annotated with 
'capability' attribute; type here is 'int'}}
+  int h;
+};
+struct C {
+  B *operator[](int);
+};
+C c;
+void f() { c[A()]->g(); }
+} // namespace PR34800


Index: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
===
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -909,15 +909,10 @@
 public:
   static bool classof(const SExpr *E) { return E->opcode() == COP_Project; }
 
-  Project(SExpr *R, StringRef SName)
-  : SExpr(COP_Project), Rec(R), SlotName(SName), Cvdecl(nullptr)
-  { }
   Project(SExpr *R, const clang::ValueDecl *Cvd)
-  : SExpr(COP_Project), Rec(R), SlotName(Cvd->getName()), Cvdecl(Cvd)
-  { }
-  Project(const Project &P, SExpr *R)
-  : SExpr(P), Rec(R), SlotName(P.SlotName), Cvdecl(P.Cvdecl)
-  { }
+  : SExpr(COP_Project), Rec(R), Cvdecl(Cvd) {
+assert(Cvd && "ValueDecl must not be null");
+  }
 
   SExpr *record() { return Rec; }
   const SExpr *record() const { return Rec; }
@@ -931,10 +926,14 @@
   }
 
   StringRef slotName() const {
-if (Cvdecl)
+if (Cvdecl->getDeclName().isIdentifier())
   return Cvdecl->getName();
-else
-  return SlotName;
+if (!SlotName) {
+  SlotName = "";
+  llvm::raw_string_ostream OS(*SlotName);
+  Cvdecl->printName(OS);
+}
+return *SlotName;
   }
 
   template 
@@ -953,7 +952,7 @@
 
 private:
   SExpr* Rec;
-  StringRef SlotName;
+  mutable llvm::Optional SlotName;
   const clang::ValueDecl *Cvdecl;
 };
 
Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5233,3 +5233,18 @@
   } // expected-warning {{mutex 'lock_' is still held at the end of function}}
   Mutex lock_ ACQUIRED_BEFORE("");
 };
+
+namespace PR34800 {
+struct A {
+  operator int() const;
+};
+struct B {
+  bool g() __attribute__((locks_excluded(h))); // expected-warning {{'locks_excluded' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'int'}}
+  int h;
+};
+struct C {
+  B *operator[](int);
+};
+C c;
+void f() { c[A()]->g(); }
+} // namespace PR34800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38549: [clang-tidy] Link targets and Asm parsers

[Alexander Kornienko via Phabricator via cfe-commits]

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

The first attempt to fix this (https://reviews.llvm.org/D17981) was stuck on 
figuring out whether we care about the binary size. It seems to me that the 
binary size is secondary to maintaining proper functionality for platforms that 
use inline assembly in headers. Thus, LG


https://reviews.llvm.org/D38549



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

[PATCH] D38284: [clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:97
+  }
+  while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
+ !Tok.is(tok::l_brace)) {

The check started triggering an assertion failure and incredible slowness 
(infinite loops?) on some real files. I've not yet come up with an isolated 
test case, but all this seems to be happening around this loop.

I'm going to revert this revision. A bug report (and hopefully a test case ;) 
will follow.



https://reviews.llvm.org/D38284



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

[PATCH] D38284: [clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:97
+  }
+  while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
+ !Tok.is(tok::l_brace)) {

alexfh wrote:
> The check started triggering an assertion failure and incredible slowness 
> (infinite loops?) on some real files. I've not yet come up with an isolated 
> test case, but all this seems to be happening around this loop.
> 
> I'm going to revert this revision. A bug report (and hopefully a test case ;) 
> will follow.
> 
Reverted in r315580.


https://reviews.llvm.org/D38284



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

[PATCH] D38284: [clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:97
+  }
+  while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
+ !Tok.is(tok::l_brace)) {

alexfh wrote:
> alexfh wrote:
> > The check started triggering an assertion failure and incredible slowness 
> > (infinite loops?) on some real files. I've not yet come up with an isolated 
> > test case, but all this seems to be happening around this loop.
> > 
> > I'm going to revert this revision. A bug report (and hopefully a test case 
> > ;) will follow.
> > 
> Reverted in r315580.
Here's a test case that demonstrates the issue:
```
#define MACRO macro_expansion
namespace MACRO {
void f(); // So that the namespace isn't empty.
// 1
// 2
// 3
// 4
// 5
// 6
// 7
// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'macro_expansion' not 
terminated with
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts 
here
}
// CHECK-FIXES: }  // namespace macro_expansion

```

I'll commit it once Subversion on llvm.org starts working again.


https://reviews.llvm.org/D38284



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

[PATCH] D38284: [clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:97
+  }
+  while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
+ !Tok.is(tok::l_brace)) {

alexfh wrote:
> alexfh wrote:
> > alexfh wrote:
> > > The check started triggering an assertion failure and incredible slowness 
> > > (infinite loops?) on some real files. I've not yet come up with an 
> > > isolated test case, but all this seems to be happening around this loop.
> > > 
> > > I'm going to revert this revision. A bug report (and hopefully a test 
> > > case ;) will follow.
> > > 
> > Reverted in r315580.
> Here's a test case that demonstrates the issue:
> ```
> #define MACRO macro_expansion
> namespace MACRO {
> void f(); // So that the namespace isn't empty.
> // 1
> // 2
> // 3
> // 4
> // 5
> // 6
> // 7
> // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'macro_expansion' not 
> terminated with
> // CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts 
> here
> }
> // CHECK-FIXES: }  // namespace macro_expansion
> 
> ```
> 
> I'll commit it once Subversion on llvm.org starts working again.
Committed the regression test in r315682.


https://reviews.llvm.org/D38284



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

[PATCH] D38549: [clang-tidy] Link targets and Asm parsers

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Zachary, do you plan to commit this? I'd also suggest adding a regression test.

Thanks!


https://reviews.llvm.org/D38549



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

[PATCH] D39105: Patch bug 27628 (clang-tidy should exit with a failure code when there are errors)

[Alexander Kornienko via Phabricator via cfe-commits]

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

Looks good! Thank you!


https://reviews.llvm.org/D39105



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

[PATCH] D38549: [clang-tidy] Link targets and Asm parsers

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp:13-16
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+  }

Will this compile on linux and mac? If no, maybe a separate test just for 
windows is the way to go.


https://reviews.llvm.org/D38549



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

[PATCH] D38549: [clang-tidy] Link targets and Asm parsers

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp:13-16
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+  }

alexfh wrote:
> Will this compile on linux and mac? If no, maybe a separate test just for 
> windows is the way to go.
... answering my own question: no, it doesn't compile on linux 
(https://godbolt.org/g/9HVvjp).

Please add a separate test guarded by `REQUIRES: system-windows` and ensure it 
breaks before your patch and passes with it.


https://reviews.llvm.org/D38549



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

[PATCH] D39105: Patch bug 27628 (clang-tidy should exit with a failure code when there are errors)

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

This patch breaks a number of tests. Please run the tests and fix the failures.


https://reviews.llvm.org/D39105



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

[PATCH] D39105: Patch bug 27628 (clang-tidy should exit with a failure code when there are errors)

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/tool/ClangTidyMain.cpp:452
 
+  if (FoundErrors) {
+  return 1;

Failed tests reminded me of the -fix-errors flag.

I suppose, clang-tidy should not return an error code, when all errors it 
noticed, contained fixes that were successfully applied by -fix-errors.


https://reviews.llvm.org/D39105



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Apart from all other comments, I think, this check would better be placed under 
bugprone/.


Repository:
  rL LLVM

https://reviews.llvm.org/D39121



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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp:607
+AnalysisDeclContextManager::~AnalysisDeclContextManager() {
+  if (!BdyFrm)
+delete BdyFrm;

This is an almost guaranteed memory leak. I think, you meant `if (BdyFrm) 
delete BdyFrm;`. But `delete` deals well with `nullptr`, so just delete the 
pointer unconditionally. But first consider making `BdyFrm` a 
`std::unique_ptr`. From a cursory look I don't see any reasons not to.


Repository:
  rL LLVM

https://reviews.llvm.org/D39208



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

[PATCH] D39015: [Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: cfe/trunk/lib/Analysis/BodyFarm.cpp:366
+M.makeLvalueToRvalue(D->getParamDecl(i),
+ /* RefersToEnclosingVariableOrCapture= */ false));
 

Remove the spaces inside the comment. `/*ParamName=*/value` is the format that 
is understood by clang-format and the 
https://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html 
clang-tidy check. Same elsewhere in this patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D39015



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

[PATCH] D39201: [Analyzer] Handle implicit function reference in bodyfarming std::call_once

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: cfe/trunk/test/Analysis/call_once.cpp:298
+  param = 42;
+};
+void test_implicit_funcptr() {

Any reason for the stray semicolon here?


Repository:
  rL LLVM

https://reviews.llvm.org/D39201



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

[PATCH] D39220: [Analyzer] Store BodyFarm in std::unique_ptr

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: include/clang/Analysis/AnalysisDeclContext.h:424
   /// methods during the analysis.
-  BodyFarm *BdyFrm = nullptr;
+  std::unique_ptr BdyFrm;
 

BdyFrm is somewhat cryptic. Maybe Farm, Bodies or something else that  is not 
so weirdly abbreviated?


https://reviews.llvm.org/D39220



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

[PATCH] D39015: [Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: cfe/trunk/lib/Analysis/BodyFarm.cpp:366
+M.makeLvalueToRvalue(D->getParamDecl(i),
+ /* RefersToEnclosingVariableOrCapture= */ false));
 

george.karpenkov wrote:
> alexfh wrote:
> > Remove the spaces inside the comment. `/*ParamName=*/value` is the format 
> > that is understood by clang-format and the 
> > https://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html 
> > clang-tidy check. Same elsewhere in this patch.
> yeah, i can do that.
> can we also consider changing `clang-tidy` to ignore those spaces?
I've just looked at the source code, and, apparently, clang-tidy already 
understands spaces in argument comments already. Sorry for misinforming you. 
However, clang-format is more strict here and there's also the consistency 
argument (see the comment on r316539), which I find to be a good motivation to 
change this (or at least to not introduce more inconsistency).


Repository:
  rL LLVM

https://reviews.llvm.org/D39015



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

[PATCH] D39220: [Analyzer] Store BodyFarm in std::unique_ptr

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: include/clang/Analysis/AnalysisDeclContext.h:424
   /// methods during the analysis.
-  BodyFarm *BdyFrm = nullptr;
+  std::unique_ptr BdyFrm;
 

george.karpenkov wrote:
> alexfh wrote:
> > BdyFrm is somewhat cryptic. Maybe Farm, Bodies or something else that  is 
> > not so weirdly abbreviated?
> But that's just forced on us by the naming convention, isn't it? I think 
> other names are worse though: they don't tell us which class we can look at 
> to figure out what is it doing.
LLVM Coding Standards doesn't endorse this kind of an abbreviation, on the 
opposite:
"We cannot stress enough how important it is to use descriptive names. ... 
Avoid abbreviations unless they are well known." 
(http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
 Neither "Bdy" nor "Frm" are well known or otherwise clear to the reader. "Frm" 
could also stand for "Form" or "Frame", but in these cases it would also be 
confusing.

If you consider shorter names ("Bodies" or "Farm") non-informative, we can go 
the other direction, e.g. "FunctionBodyFarm".


Repository:
  rL LLVM

https://reviews.llvm.org/D39220



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

[PATCH] D49800: [clang-tidy: modernize] modernize-redundant-void-arg crashes when a function body is in a macro

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Thank you for working on this!




Comment at: clang-tidy/modernize/RedundantVoidArgCheck.cpp:241
+SourceLocation End =
+Lambda->getBody()->getLocStart().isMacroID()
+? Result.SourceManager

Let's pull `Lambda->getBody()->getLocStart()` to a variable to avoid repetition.



Comment at: clang-tidy/modernize/RedundantVoidArgCheck.cpp:242-244
+? Result.SourceManager
+  
->getImmediateExpansionRange(Lambda->getBody()->getLocStart())
+  .getBegin()

The fix looks limited to the specific test case.  Let's try other cases (see 
the comment below).



Comment at: test/clang-tidy/modernize-redundant-void-arg.cpp:454
+ // CHECK-FIXES: []()BODY;
+}

I'd like to see more tests here with different macro-related cases. These come 
to mind, but if you come up with more distinct cases, that would be even better:

  #define LAMBDA1 [](void){}
  (void)LAMBDA1;
  #define LAMBDA2 [](void)BODY
  (void)LAMBDA2;
  #define LAMBDA3(captures, args, body) [captures](args){body}
  (void)LAMBDA3(=, void, BODY);
  #define LAMBDA4(captures, args, body) captures args body
  (void)LAMBDA4([], (void), BODY);
  #define LAMBDAS1 \
(void)LAMBDA1; \
(void)LAMBDA2; \
(void)LAMBDA3(=, void, BODY); \
(void)LAMBDA4([], (void), BODY)

  LAMBDAS1;
  #define WRAP(...) __VA_ARGS__
  WRAP((void)WRAP(WRAP(LAMBDA4(WRAP([]), WRAP((void)), WRAP(BODY);
  (void)WRAP([](void){});




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49800



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

[PATCH] D49918: [clang-tidy] Sequence declaration in while statement before the condition

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG modulo outstanding comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49918



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

[PATCH] D50060: [clang-tidy] add all clang-tidy modules to plugin

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50060



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

[PATCH] D49890: Clang-Tidy Export Problem

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Could you describe the specific problem you're solving and provide an example? 
As mentioned by others, a test would be very welcome as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49890



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

[PATCH] D49918: [clang-tidy] Sequence init statements, declarations, and conditions correctly in if, switch, and while

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.

Still LG




Comment at: test/clang-tidy/bugprone-use-after-move.cpp:1141
+A a1;
+if (A a2= std::move(a1)) {
+  std::move(a2);

nit: clang-format this, please.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49918



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

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from header which are deprecated and removed in C++17

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG




Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > alexfh wrote:
> > > > > > > > > alexfh wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > > > > > ``? Just like we have a "deprecated 
> > > > > > > > > > > > > > headers" check, perhaps this should be a 
> > > > > > > > > > > > > > "deprecated APIs" check?
> > > > > > > > > > > > > Added full stop.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > > > >  or be extended to a full 'deprecated 
> > > > > > > > > > > > > API' check.
> > > > > > > > > > > > > This change is just a start, several more types and 
> > > > > > > > > > > > > classes which are removed from  will 
> > > > > > > > > > > > > follow, e.g:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > > > > > 
> > > > > > > > > > > > > As these are a bunch of functions and types, in my 
> > > > > > > > > > > > > eyes a check just for  is fine. But I'm 
> > > > > > > > > > > > > also fine with a general 'deprecated API' check.
> > > > > > > > > > > > > Alex, can you comment on this?
> > > > > > > > > > > > There are already other checks for functions which are 
> > > > > > > > > > > > removed in C++17 like modernize-replace-random-shuffle.
> > > > > > > > > > > > So I think having an separate check for functions and 
> > > > > > > > > > > > types removed from  would be OK.
> > > > > > > > > > > You've hit the nail on the head for what I'm trying to 
> > > > > > > > > > > avoid -- we shouldn't have multiple checks unless they do 
> > > > > > > > > > > drastically different things. Having a deprecated check 
> > > > > > > > > > > like this really only makes sense for APIs that are 
> > > > > > > > > > > deprecated but aren't uniformly marked as 
> > > > > > > > > > > `[[deprecated]]` by the library. As such, I think we 
> > > > > > > > > > > really only need one check for this rather than splitting 
> > > > > > > > > > > it out over multiple checks -- the existing check 
> > > > > > > > > > > functionality could be rolled into this one and its check 
> > > > > > > > > > > become an alias.
> > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > > check.
> > > > > > > > > > 
> > > > > > > > > > IIUC, it should be possible to implement fixits at least 
> > > > > > > > > > for some use cases here. My impression was that Jens was at 
> > > > > > > > > > least considering to work on fixits. The other check 
> > > > > > > > > > mentioned here - `modernize-replace-random-shuffle` already 
> > > > > > > > > > implements fixits. Fixits are specific to the API and some 
> > > > > > > > > > codebases may have better replacement APIs than what the 
> > > > > > > > > > standard suggests, so different users may want to apply 
> > > > > > > > > > different set of the fixes. Given all that, I wouldn't just 
> > > > > > > > > > merge all of the checks dealing with deprecated APIs. 
> > > > > > > > > > Splitting them at least by header seems like a good start, 
> > > > > > > > > > maybe even finer granularity may be needed in some cases.
> > > > > > > > > TL;DR "they do drastically different things" is the case for 
> > > > > > > > > this check and modernize-replace-random-shuffle.
> > > > > > > > I disagree that they do drastically different things or that 
> > > > > > > > fix-its are a problem. Some of these APIs have replacements, 
> > > > > > > > others do not. At the end of the day, the basics are the same: 
> > > > > > > > the functionality is deprecated and 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from header which are deprecated and removed in C++17

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:50
+CheckFactories.registerCheck(
+"modernize-avoid-functional");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> I'm not keen on this name -- it suggests the user should avoid functional, 
> which is certainly not the advice we want to give them. Also, it makes no 
> mention of deprecations, which is what the check is all about.
> 
> How about: `modernize-functional-deprecations` and we can use 
> `modernize-deprecations` as the eventual catch-all umbrella, 
> `modernize--deprecations` for other headers, and 
> `modernize--deprecations-` if we want to add API-level 
> granularity?
I'd suggest to put "deprecations" first: modernize-deprecations-functional. Or 
maybe modernize-deprecated-functional?


https://reviews.llvm.org/D42730



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a reviewer: ioeric.
alexfh added a comment.

In https://reviews.llvm.org/D43500#1013497, @aaron.ballman wrote:

> > It seems, when they are apply directly from clang-tidy, the RefactoringTool 
> > engine is smart enough to remove trailing tokens. However, when fixes are 
> > exported, clang-apply-replacements cannot do the same trick and will lead 
> > trailing commas and colon
>
> Is there a way to make clang-apply-replacements smarter rather than forcing 
> every check to jump through hoops? I'm worried that if we have to fix 
> individual checks we'll just run into the same bug later.


Eric was going to look at this, IIRC.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43500



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

[PATCH] D33531: Clang-tidy readability: avoid const value return

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

> The check ignores returns of const pointers (meaning * const, not const *); 
> while pointless, these are not particularly harmful. It could be made laxer 
> by ignoring all trivially copyable types (on the grounds that they won't have 
> interesting move constructors/assigners anyway), or stricter by ignoring all 
> const returns (on the grounds that, in the best case, it's just pointless 
> verbosity). Or it could be made an option.

I'd prefer an option. It could be `StrictMode`, which is also supported as a 
global option, for example, by clang-tidy/misc/ArgumentCommentCheck.cpp.


Repository:
  rL LLVM

https://reviews.llvm.org/D33531



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

[PATCH] D33531: Clang-tidy readability: avoid const value return

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:27
+  // skip those too.
+  Finder->addMatcher(functionDecl(returns(qualType(
+isConstQualified(),

How about just matching definitions to avoid duplicate warnings?



Comment at: test/clang-tidy/readability-const-value-return.cpp:1
+// RUN: %check_clang_tidy %s readability-const-value-return %t
+

Please add tests for:
1. a function with multiple declarations and a definition
2. function declarations/definitions in macros
3. a class method
4. implicit stuff (I'm not sure if a lambda can be made to return a const type 
without an explicit return type specification)



Comment at: test/clang-tidy/readability-const-value-return.cpp:50
+template 
+const T f_returns_template_param();
+

It would be nice to ensure the check doesn't trigger on template instantiation 
of this function as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D33531



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

[PATCH] D33531: Clang-tidy readability: avoid const value return

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

> Would it make sense to silence this diagnostic in the presence of also 
> checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or 
compatibility issues between checks. Moreover, we already have checks that are 
not meant to be run together, for example, readability-braces-around-statements 
and its google- incarnation (and other alias checks with different settings). 
That said, we could whitelist postfix increment and decrement operators in this 
check. Camillo, WDYT?

On a side note, the check's performance implications might be more important 
than the readability aspect of dropping the `const`, so the check might be a 
better fit for the `performance` category.


Repository:
  rL LLVM

https://reviews.llvm.org/D33531



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

[PATCH] D33623: Make the parser close parens for you on EOF

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Sam knows this stuff much better.


https://reviews.llvm.org/D33623



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/misc/LambdaFunctionNameCheck.cpp:61-62
+void LambdaFunctionNameCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Compiler.getPreprocessor().addPPCallbacks(std::unique_ptr(
+  new MacroExpansionsWithFileAndLine(&SuppressMacroExpansions)));
+}

`llvm::make_unique(...)`



Comment at: clang-tidy/misc/LambdaFunctionNameCheck.cpp:67-68
+  const auto *E = Result.Nodes.getNodeAs("E");
+  if (E->getIdentType() == PredefinedExpr::Func ||
+  E->getIdentType() == PredefinedExpr::Function) {
+if (E->getLocation().isMacroID()) {

nit: I'd use the early return style here.



Comment at: clang-tidy/misc/LambdaFunctionNameCheck.cpp:73-74
+  SourceRange AsRange(ER.first, ER.second);
+  for (const auto& R : SuppressMacroExpansions) {
+if (R == AsRange) {
+  // This is a macro expansion for which we should not warn.

I'm afraid this can become extremely slow on large files with 
boilerplate/generated code. Since the code is just looking for exact matches 
(and not any overlapping ranges, for example), we could replace the vector with 
a (hash?) map to limit the worst case complexity.


https://reviews.llvm.org/D33497



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

[PATCH] D33531: Clang-tidy readability: avoid const value return

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D33531#767628, @aaron.ballman wrote:

> In https://reviews.llvm.org/D33531#767059, @alexfh wrote:
>
> > > Would it make sense to silence this diagnostic in the presence of also 
> > > checking for cert-dcl21-cpp for such operators?
> >
> > Currently there's no mechanism in clang-tidy to express dependencies or 
> > compatibility issues between checks. Moreover, we already have checks that 
> > are not meant to be run together, for example, 
> > readability-braces-around-statements and its google- incarnation (and other 
> > alias checks with different settings). That said, we could whitelist 
> > postfix increment and decrement operators in this check. Camillo, WDYT?
>
>
> I can imagine a generic whitelist mechanism might be useful for this check. 
> It could even be empty by default, but the documentation could call out 
> cert-dcl21-cpp specifically and show an example of how you can run both 
> checks.


A generic whitelist of method/function names would make sense, if we had more 
use cases for it. It might also be quite tricky to implement: distinguishing 
between prefix and postfix increment/decrement operators would require 
specifying arguments, and allowing it for all types would need a support for 
pattern matching or optional omission of the type name on methods. All this 
seems to be an overkill so far. If we want this whitelisting be optional, we 
can add a boolean option specifically for these operators.


Repository:
  rL LLVM

https://reviews.llvm.org/D33531



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

[PATCH] D33304: [clang-tidy] Add a new module Android and three new checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

It's awesome to see another two checks, and it seems to justify adding a 
separate module, but I'd prefer the other two checks to be sent for review as 
separate patches to make the review easier and faster.


https://reviews.llvm.org/D33304



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

[PATCH] D33304: [clang-tidy] Add a new module Android and three new checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D33304#768961, @yawanng wrote:

> In https://reviews.llvm.org/D33304#768731, @alexfh wrote:
>
> > It's awesome to see another two checks, and it seems to justify adding a 
> > separate module, but I'd prefer the other two checks to be sent for review 
> > as separate patches to make the review easier and faster.
>
>
> The other two checks are relatively small and simple. I understand this may 
> make the review process longer and we could wait for that. Please take your 
> time :-) Thank you!


It's not only longer to review larger patches, it's also less convenient for 
the reviewers. Please send each check as a separate patch (the addition of the 
module can be in one of them). Phabricator has decent support for dependent 
patches, so it shouldn't add much work on your side.


https://reviews.llvm.org/D33304



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

[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D33304#771215, @hokein wrote:

> Thanks for the contributions.
>
> All your three checks are not android specific -- because they are checking 
> POSIX APIs (`open`, `creat`, `fopen`), which are more likely related to 
> POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but 
> wait to see other reviewers' opinions before renaming it.


IIUC, these checks enforce a certain - Android-specific - way of using POSIX 
APIs. I'm not sure if the recommendations are universally useful. Or am I 
mistaken?


https://reviews.llvm.org/D33304



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

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

Looks good!


https://reviews.llvm.org/D33497



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL304570: [clang-tidy] check for __func__/__FUNCTION__ in 
lambdas (authored by alexfh).

Changed prior to commit:
  https://reviews.llvm.org/D33497?vs=101224&id=101237#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33497

Files:
  clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.cpp
  clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.h
  clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/misc-lambda-function-name.rst
  clang-tools-extra/trunk/test/clang-tidy/misc-lambda-function-name.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/misc-lambda-function-name.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/misc-lambda-function-name.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/misc-lambda-function-name.cpp
@@ -0,0 +1,41 @@
+// RUN: %check_clang_tidy %s misc-lambda-function-name %t
+
+void Foo(const char* a, const char* b, int c) {}
+
+#define FUNC_MACRO Foo(__func__, "", 0)
+#define FUNCTION_MACRO Foo(__FUNCTION__, "", 0)
+#define EMBED_IN_ANOTHER_MACRO1 FUNC_MACRO
+
+void Positives() {
+  [] { __func__; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__func__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { __FUNCTION__; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__FUNCTION__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { FUNC_MACRO; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__func__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { FUNCTION_MACRO; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__FUNCTION__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { EMBED_IN_ANOTHER_MACRO1; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__func__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+}
+
+#define FUNC_MACRO_WITH_FILE_AND_LINE Foo(__func__, __FILE__, __LINE__)
+#define FUNCTION_MACRO_WITH_FILE_AND_LINE Foo(__FUNCTION__, __FILE__, __LINE__)
+#define EMBED_IN_ANOTHER_MACRO2 FUNC_MACRO_WITH_FILE_AND_LINE
+
+void Negatives() {
+  __func__;
+  __FUNCTION__;
+
+  // __PRETTY_FUNCTION__ should not trigger a warning because its value is
+  // actually potentially useful.
+  __PRETTY_FUNCTION__;
+  [] { __PRETTY_FUNCTION__; }();
+
+  // Don't warn if __func__/__FUNCTION is used inside a macro that also uses
+  // __FILE__ and __LINE__, on the assumption that __FILE__ and __LINE__ will
+  // be useful even if __func__/__FUNCTION__ is not.
+  [] { FUNC_MACRO_WITH_FILE_AND_LINE; }();
+  [] { FUNCTION_MACRO_WITH_FILE_AND_LINE; }();
+  [] { EMBED_IN_ANOTHER_MACRO2; }();
+}
Index: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
@@ -4,6 +4,7 @@
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
+  LambdaFunctionNameCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
@@ -21,6 +21,7 @@
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
+#include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedConstCheck.h"
@@ -68,6 +69,8 @@
 "misc-assert-side-effect");
 CheckFactories.registerCheck(
 "misc-forwarding-reference-overload");
+CheckFactories.registerCheck(
+"misc-lambda-function-name");
 CheckFactories.registerCheck("misc-misplaced-const");
 CheckFactories.registerCheck(
 "misc-unconventional-assign-operator");
Index: clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.cpp
===
--- 

[PATCH] D33827: [clang-tidy] misc-static-assert shouldn't flag assert(!"msg")

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG. Thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D33827



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D20693#774693, @hintonda wrote:

> In order to fix diagnostic corruption in some of the buildbot tests
>  (unable to reproduce locally):
>
> - make NoexceptMacro a static variable so it's lifetime doesn't end when 
> UseNoexceptCheck is destroyed.
> - pass a const char* instead of a StringRef to DiagnosticBuilder so it won't 
> create a temporary std::string and cache the address of the temporary char * 
> it owns.


That's pretty hacky, even if this fixes the immediate issue (which I'm somewhat 
skeptical about). Do you have failure logs saved somewhere, by chance? If not, 
I'd suggest to resubmit the original patch (without these two hacks) and grab 
the fresh failure logs. Another suggestion is to run the tests with asan (and 
maybe with msan) to see, whether the issue can be detected by the sanitizers.


https://reviews.llvm.org/D20693



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

In any case, I'm strongly against these hacks, please revert them.


https://reviews.llvm.org/D20693



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

[PATCH] D33982: [clang-format] Fix alignment of preprocessor trailing comments

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG




Comment at: lib/Format/WhitespaceManager.cpp:112
+assert(PreviousOriginalWhitespaceEndOffset <= 
OriginalWhitespaceStartOffset);
+StringRef Text(SourceMgr.getCharacterData(PreviousOriginalWhitespaceEnd),
+   SourceMgr.getCharacterData(OriginalWhitespaceStart) -

I'd pull `SourceMgr.getCharacterData(PreviousOriginalWhitespaceEnd)` out to a 
variable.


https://reviews.llvm.org/D33982



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D20693#775014, @hintonda wrote:

> - Only pass %2 parameter if %2 is included in format.


I thought, DiagnosticsBuilder handles placeholders in conditional parts 
correctly. Did you find an evidence of the opposite? Can you add a test that 
consistently fails?




Comment at: clang-tidy/modernize/UseNoexceptCheck.h:42
+  const std::string NoexceptMacro;
+  bool UseNoexceptFalse;
+};

This can also be `const` for consistency with how options are handled in this 
and other checks.


https://reviews.llvm.org/D20693



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D20693#775153, @alexfh wrote:

> In https://reviews.llvm.org/D20693#775014, @hintonda wrote:
>
> > - Only pass %2 parameter if %2 is included in format.
>
>
> I thought, DiagnosticsBuilder handles placeholders in conditional parts 
> correctly. Did you find an evidence of the opposite? Can you add a test that 
> consistently fails?


(without your latest change that is)


https://reviews.llvm.org/D20693



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D20693#775197, @hintonda wrote:

> I have not, as yet, been able to reproduce the buildbot failures.  They were 
> essentially intermittent seg-faults, and corrupt diag output.
>
> I will work on creating a test that can reproduce the problem.


As I said, we could re-submit the patch (without speculative fixes) and see 
whether the buildbots break again. One more suggestion is to run the tests with 
asan. Have you tried this?


https://reviews.llvm.org/D20693



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

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



Comment at: docs/clang-tidy/checks/readability-redundant-keyword.rst:8
+
+`extern` is redundant in function declarations
+

Could you explain, why you think `extern` is redundant in function declarations?


Repository:
  rL LLVM

https://reviews.llvm.org/D33841



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

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

Now that you found the root cause, this looks like a proper fix ;)

Thank you! LG


https://reviews.llvm.org/D20693



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

[PATCH] D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

IIUC, when `vector` (for a class `T` that has both move and copy 
constructors) resizes, it will prefer move constructors, but only if they're 
declared `noexcept`.  This is true even if `-fno-exceptions` is on. So I don't 
think this check should depend on `-fno-exceptions`.


https://reviews.llvm.org/D34002



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

[PATCH] D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Here's a small test: https://godbolt.org/g/nNZgUb (look for `T::T(` and `S::S(` 
calls, adding `-fno-exceptions` doesn't change which constructors are called).


https://reviews.llvm.org/D34002



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I would be interested in seeing the results of this check's run on LLVM+Clang 
code.


https://reviews.llvm.org/D33722



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

[PATCH] D33365: [clang-tidy] misc-assertion-count: A New Check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

I guess, this check should go to `readability` or elsewhere, but definitely not 
to `misc`.

Another big question is whether it's reasonable to set up specific ratio limits 
on the density of asserts. I think, density of asserts strongly depends on the 
nature of the code, and there is no single answer to how much asserts should be 
used. IIUC, neither of the recommendations you mentioned contain any 
quantitative measures, they just delegate the decision to the developer. I'm 
not saying it's impossible to find good formalization of these rules, but I'd 
expect some sort of analysis of existing codebases with regard to how asserts 
are used (not just the density of asserts, but also positioning of asserts and 
what is being checked by the asserts) in different types of code.


Repository:
  rL LLVM

https://reviews.llvm.org/D33365



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

[PATCH] D33365: [clang-tidy] misc-assertion-count: A New Check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D33365#775880, @lebedev.ri wrote:

> In https://reviews.llvm.org/D33365#775860, @alexfh wrote:
>
> > I guess, this check should go to `readability` or elsewhere, but definitely 
> > not to `misc`.
>
>
> Hmm, `misc` may be a bad place for this, but i think `readability` is even 
> worse fit.
>  The best guess would be something like `hardening` / `security`, but there 
> is no such category.


`readability` might be reasonable, since one of the most important functions of 
asserts is documenting invariants. The second function is enforcing invariants, 
but in correct code asserts are basically no-ops, and they fire only when the 
code is being changed and the invariants become broken.

>> Another big question is whether it's reasonable to set up specific ratio 
>> limits on the density of asserts. I think, density of asserts strongly 
>> depends on the nature of the code, and there is no single answer to how much 
>> asserts should be used. IIUC, neither of the recommendations you mentioned 
>> contain any quantitative measures, they just delegate the decision to the 
>> developer.
> 
> No, it is not reasonable to set up **default** ratio limits on the density of 
> asserts.
>  That is exactly why the default params are NOP, and i even made sure that if 
> the params are NOP, this check will not add any overhead (no PPCallback, no 
> matchers).
> 
> Did that answer your question?

No, I'm not talking about default ratio limits. I wonder whether there's any 
specific combination of the options that would serve well to the needs of any 
real project. I suspect that the number of lines in a function alone is 
insufficient to determine reasonable limits on the number of asserts in it. I 
guess, if it's even possible to find out the number of invariants that is 
valuable to enforce in a certain function, a much larger set of inputs should 
be considered. One little example is that many functions check whether their 
pointer arguments are not null. Thus, a function that has no pointer arguments 
will naturally require fewer asserts.

The only relevant formalized rule I found, is the JPL's rule 16 you refer to 
(functions with more than 10 LOCs should have at least one assertion). But it 
doesn't go further and say how many assertions should be in a function with, 
say, 30 lines of code (I think, because it would be rather difficult to 
formalize all the different situations).

I think, this kind of a check needs some prior research (not necessarily in the 
sense of a printed paper, but at least a thoughtful analysis of the relevant 
metrics on real code bases)  to back up the specific way the sufficiency of 
asserts is determined.

>> I'm not saying it's impossible to find good formalization of these rules, 
>> but I'd expect some sort of analysis of existing codebases with regard to 
>> how asserts are used (not just the density of asserts, but also positioning 
>> of asserts and what is being checked by the asserts) in different types of 
>> code.




Repository:
  rL LLVM

https://reviews.llvm.org/D33365



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D20693#776030, @hintonda wrote:

> Great, thanks for you help.
>
> Could you commit this for me?


Sure, running tests...


https://reviews.llvm.org/D20693



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

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL304977: [clang-tidy] New checker to replace dynamic 
exception specifications (authored by alexfh).

Changed prior to commit:
  https://reviews.llvm.org/D20693?vs=101860&id=101910#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D20693

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseNoexceptCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseNoexceptCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-noexcept.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-noexcept-macro.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-noexcept-opt.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-noexcept.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/modernize-use-noexcept-opt.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-noexcept-opt.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-noexcept-opt.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy %s modernize-use-noexcept %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-noexcept.UseNoexceptFalse, value: 0}]}" \
+// RUN:   -- -std=c++11
+
+class A {};
+class B {};
+
+void foo() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-FIXES: void foo() noexcept;
+
+void bar() throw(...);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw(...)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void bar() ;
+
+void k() throw(int(int));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: dynamic exception specification 'throw(int(int))' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void k() ;
+
+void foobar() throw(A, B)
+{}
+// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: dynamic exception specification 'throw(A, B)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void foobar()
+
+void baz(int = (throw A(), 0)) throw(A, B) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: dynamic exception specification 'throw(A, B)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void baz(int = (throw A(), 0)) {}
+
+void g(void (*fp)(void) throw());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-FIXES: void g(void (*fp)(void) noexcept);
+
+void f(void (*fp)(void) throw(int)) throw(char);
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: dynamic exception specification 'throw(int)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: dynamic exception specification 'throw(char)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void f(void (*fp)(void) ) ;
+
+#define THROW throw
+void h(void (*fp)(void) THROW(int)) THROW(char);
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: dynamic exception specification 'THROW(int)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: dynamic exception specification 'THROW(char)' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void h(void (*fp)(void) ) ;
+
+void j() throw(int(int) throw(void(void) throw(int)));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: dynamic exception specification 'throw(int(int) throw(void(void) throw(int)))' is deprecated; consider removing it instead [modernize-use-noexcept]
+// CHECK-FIXES: void j() ;
+
+class Y {
+  Y() throw() = default;
+};
+// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-FIXES: Y() noexcept = default;
+
+struct Z {
+  void operator delete(void *ptr) throw();
+  void operator delete[](void *ptr) throw(int);
+  ~Z() throw(int) {}
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:35: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:37: warning: dynamic exception specification 'throw(int)' is deprecated; consider using 'noexcept(false)' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:8: warning: dynamic exception specification 'throw(int)' is deprecated; consider using 'noexcept(false)' instead [modernize-use-noexcept]
+// CHECK-FIXES: void operator delete(void *ptr)

[PATCH] D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D34002#776193, @chh wrote:

> In https://reviews.llvm.org/D34002#775830, @alexfh wrote:
>
> > IIUC, when `vector` (for a class `T` that has both move and copy 
> > constructors) resizes, it will prefer move constructors, but only if 
> > they're declared `noexcept`.  This is true even if `-fno-exceptions` is on. 
> > So I don't think this check should depend on `-fno-exceptions`.
>
>
> Should the compiler assume `noexcept` when -fno-exceptions is on?
>  That means move constructors should be preferred under -fno-exceptions, and 
> this check would be unnecessary, right?


The compiler doesn't assume `noexcept` and I heard from competent people the 
reasons why it shouldn't, though I can't immediately recall these reasons. I 
think, the patch should be reverted.


https://reviews.llvm.org/D34002



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

[PATCH] D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: test/clang-tidy/misc-noexcept-move-constructor.cpp:2
+// RUN: clang-tidy %s -checks="-*,misc-noexcept-move-constructor" -- 
-std=c++11 \
+// RUN:   | FileCheck %s -check-prefix=CHECK-EXCEPTIONS \
+// RUN:   -implicit-check-not="{{warning|error}}:"

FlameTop wrote:
> Could I request that exceptions be explicitly enabled here (-fexceptions) as 
> the test currently fails on targets which are configured with exceptions 
> disabled as default? 
That would be the way to go, if we wanted to keep this change in. However, we 
need to revert it (which should also fix the issues you're talking about).


https://reviews.llvm.org/D34002



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

[PATCH] D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D34002#776551, @chh wrote:

> Android source is suppressing misc-noexcept-move-constructor warnings
>  because -fno-exceptions is used and Android does not like to add more
>  exception specific code.


As I've said, the lack of `noexcept` on move constructor of a type will make 
certain operations on STL containers of that type slower regardless of whether 
exceptions are turned on or off. `-fno-exceptions` will in no way help to 
recover the lost performance, so there's absolutely no reason to silence this 
check on the code that is otherwise not using exceptions.

Reverted in r305057.

> It's not a big deal for Android to suppress this check one way or the other.
>  I don't mind reverting it, just curious why a compiler cannot assume 
> noexcept under -fno-exceptions.

One reason for compiler not to treat each function as `noexcept`, when compiled 
with `-fno-exceptions` is that it would make it hard to link this code together 
with code compiled with `-fexceptions` without causing ODR violations.


https://reviews.llvm.org/D34002



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

[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.

[Alexander Kornienko via Phabricator via cfe-commits]

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



Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:60
+
+  LangOptions LangOpts = getLangOpts();
+  SourceRange FlagsRange(FlagArg->getLocStart(), FlagArg->getLocEnd());

No need for this variable, since it's just used once. Same below.



Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:62-65
+  StringRef FlagsText = Lexer::getSourceText(
+  CharSourceRange::getTokenRange(FlagsRange), SM, LangOpts);
+  std::string ReplacementText =
+  (llvm::Twine(FlagsText) + " | " + O_CLOEXEC).str();

No need to replace the text with itself. Just insert the part that is missing. 
The more local the changes - the fewer are chances of conflicts.



Comment at: clang-tidy/android/FileOpenFlagCheck.h:38
+
+  static constexpr const char *O_CLOEXEC = "O_CLOEXEC";
+};

This doesn't have to be a class member. It could be local to the function where 
it's used or to the .cc file.



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

I'm not sure about using of "has been" here. Maybe just use present tense? You 
might want to consult a nearby native speaker though. Alternatively, you might 
want to rephrase this as "Opening files using POSIX ``open`` or similar system 
calls without specifying the ``O_CLOEXEC`` flag is a common source of security 
bugs."



Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:9
+remain open across a fork+exec to a lower-privileged SELinux domain, leaking
+that sensitive data Functions including ``open()``, ``openat()``, and
+``open64()`` must include ``O_CLOEXEC`` in their flags argument.

Missing period before "Functions".



Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:9
+remain open across a fork+exec to a lower-privileged SELinux domain, leaking
+that sensitive data Functions including ``open()``, ``openat()``, and
+``open64()`` must include ``O_CLOEXEC`` in their flags argument.

alexfh wrote:
> Missing period before "Functions".
Just "functions" is too broad. I'd say "`open`-like functions", "POSIX 
functions that open files" or something like that.


https://reviews.llvm.org/D33304



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

[PATCH] D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D34002#776948, @chh wrote:

> Thanks for providing the ODR reason for Android -fno-exceptions users to 
> change source code or suppress this warning themselves.


I'm not sure I understand your comment. Could you clarify?

Here's an explanation about the possibility of an ODR violation in case the 
compiler would assume `noexcept` for all functions compiled with 
`-fno-exceptions`. Consider the following code:
a.h:

  struct A {
A(A&&);
A(const A&);
...
  };

b.cc:

  #include "a.h"
  #include 
  ...
std::vector v1;
  ...

c.cc:

  #include "a.h"
  #include 
  ...
std::vector v2;
  ...

Now if we compile b.cc with `-fexceptions` and c.cc with `-fno-exceptions`, and 
IF the compiler was changed to treat all functions in c.cc as `noexcept`, then 
the instantiation of some methods of std::vector in b.cc would use the copy 
constructor of A, but the same methods would use the move constructor of A in 
c.cc. When these two were linked together, that would be an ODR violation.


https://reviews.llvm.org/D34002



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

[PATCH] D34524: [clang-tidy] Fix a false positive in modernize-use-nullptr.

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG. Thank you for the fix!


https://reviews.llvm.org/D34524



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