[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: shuaiwang, JonasToth, lebedev.ri.
lebedev.ri added a comment.

Regression is very broad term. Is it a false-positive?
Perhaps it is better to address the issue in the ExprMutationAnalyzer itself?


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] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: EricWF.
lebedev.ri added a comment.

In https://reviews.llvm.org/D50447#1192316, @hokein wrote:

> In https://reviews.llvm.org/D50447#1192280, @JonasToth wrote:
>
> > If whitelisting works, thats fine. But I agree with @lebedev.ri that a 
> > contribution/improvement to the ExprMutAnalyzer is the better option. This 
> > is especially the case, because multiple checks (and maybe even some other 
> > parts then clang-tidy) will utilize this analysis.
>
>
> I'm sorry for not explaining it with more details. Might be "regression" is a 
> confusing world :(. It is not false positive. The change using 
> ExprMutAnalyzer is reasonable, IMO. It makes this check smarter to catch 
> cases which will not be caught before. For example,
>
>   for (auto widget : container) {
> widget.call_const_method(); // const usage recongized by ExprMutAnalyzer, 
> so it is fine to change widget to `const auto&`
>   } 
>
>
> But in our codebase, we have code intended to use like below, and it is in 
> base library, and is used widely. I think it makes sense to whitelist this 
> class in our internal configuration.
>
>   for (auto _ : state) {
>  ... // no `_` being referenced in the for-loop body
>   }
>


That looks remarkably like google benchmark main loop. (i don't know at hand if 
making this const will break it, but probably not?)
I wonder if instead there should be an option to not complain about the 
variables that aren't actually used?


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] D50447: [clang-tidy] Omit cases where loop variable is not used in loop body in performance-for-range-copy.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

It seems this ended up silently being a catch-all, with no option to control 
this behavior, and i don't see any comments discussing this..


Repository:
  rL LLVM

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] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 160106.
lebedev.ri added a comment.

Rebase (ugh, bitrot), port `test/clang-tidy/hicpp-exception-baseclass.cpp`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D36892

Files:
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/hicpp-exception-baseclass.cpp

Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -20,28 +20,28 @@
 void problematic() {
   try {
 throw int(42);
-// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (int e) {
   }
   throw int(42);
-  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
 
   try {
 throw 12;
-// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (...) {
 throw; // Ok, even if the type is not known, conforming code can never rethrow a non-std::exception object.
   }
 
   try {
 throw non_derived_exception();
-// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 9:1: note: type defined here
+// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 9:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
-  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-MESSAGES: 9:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+  // CHECK-NOTES: 9:1: note: type defined here
 
 // FIXME: More complicated kinds of inheritance should be checked later, but there is
 // currently no way use ASTMatchers for this kind of task.
@@ -100,15 +100,15 @@
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { throw T(); }
-// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 120:1: note: type defined here
-// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 120:1: note: type defined here
-// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 123:1: note: type defined here
-// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-// CHECK-MESSAGES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 9:1: note: type defined here
+// CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 120:1: note: type defined here
+// CHECK-NOTES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 120:1: note: type defined here
+// CHECK-NOTES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 123:1: note: type defined here
+// CHECK-NOTES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 9:1: note: type defined here
 #define THROW_EXCEPTION(CLASS) ThrowException()
 #define THROW_BAD_EXCEPTION throw int(42);
 #define THROW_GOOD_EXCEPTION throw std::exception();
@@ -134,25 +134,28 @@
   THROW_EXCEPTION(deep_hierarchy);// Ok
 
   THROW_BAD_EXCEPTION;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-  // CHECK-MESSAGES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived f

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 160107.
lebedev.ri added a comment.

Add docs note.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D36892

Files:
  docs/clang-tidy/index.rst
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/hicpp-exception-baseclass.cpp

Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -20,28 +20,28 @@
 void problematic() {
   try {
 throw int(42);
-// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (int e) {
   }
   throw int(42);
-  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
 
   try {
 throw 12;
-// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (...) {
 throw; // Ok, even if the type is not known, conforming code can never rethrow a non-std::exception object.
   }
 
   try {
 throw non_derived_exception();
-// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 9:1: note: type defined here
+// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 9:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
-  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-MESSAGES: 9:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+  // CHECK-NOTES: 9:1: note: type defined here
 
 // FIXME: More complicated kinds of inheritance should be checked later, but there is
 // currently no way use ASTMatchers for this kind of task.
@@ -100,15 +100,15 @@
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { throw T(); }
-// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 120:1: note: type defined here
-// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 120:1: note: type defined here
-// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 123:1: note: type defined here
-// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-// CHECK-MESSAGES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-MESSAGES: 9:1: note: type defined here
+// CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 120:1: note: type defined here
+// CHECK-NOTES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 120:1: note: type defined here
+// CHECK-NOTES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 123:1: note: type defined here
+// CHECK-NOTES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 9:1: note: type defined here
 #define THROW_EXCEPTION(CLASS) ThrowException()
 #define THROW_BAD_EXCEPTION throw int(42);
 #define THROW_GOOD_EXCEPTION throw std::exception();
@@ -134,25 +134,28 @@
   THROW_EXCEPTION(deep_hierarchy);// Ok
 
   THROW_BAD_EXCEPTION;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-  // CHECK-MESSAGES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  // CHECK-

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

> We are aware that this test will cause warnings on users code through their 
> dependencies on abseil.
>  However, from what we know it seems like these warnings are normally 
> suppressed.
>  If anyone has a good idea on how to avoid this/has insight on whether this 
> will be a problem for the average user, please let me know!

Suppressed how?
I think this needs tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50580



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I suspect this might deserve a wider discussion. At least cfe-dev, perhaps?


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 160458.
lebedev.ri added a comment.

Ping.

Rebased, now that the https://reviews.llvm.org/D50465 has landed, and we are 
now able to properly optimize the ugliest case:

> This comes with `Implicit Conversion Sanitizer - integer sign change` 
> (https://reviews.llvm.org/D50250):
> 
>   signed char test(unsigned int x) { return x; }
> 
> 
> `clang++ -fsanitize=implicit-conversion -S -emit-llvm -o - /tmp/test.cpp -O3`
> 
> - Old: F6904292: old.ll 
> - With this patch: F6904294: new.ll 


Repository:
  rC Clang

https://reviews.llvm.org/D50250

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/catch-implicit-integer-conversions-basics.c
  test/CodeGen/catch-implicit-integer-conversions.c
  test/CodeGen/catch-implicit-integer-sign-changes-basics.c
  test/CodeGen/catch-implicit-integer-sign-changes-true-negatives.c
  test/CodeGen/catch-implicit-integer-sign-changes.c
  test/CodeGen/catch-implicit-integer-truncations-basics.c
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-sign-changes-true-negatives.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,22 +29,22 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation),?){6}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation|implicit-integer-sign-change),?){7}"}}
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
-// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation|implicit-integer-sign-change),?){2}"}}
+// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation|implicit-integer-sign-change),

[PATCH] D50709: [clang-doc] Fix unused var

[Roman Lebedev via Phabricator via cfe-commits]

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

Looks like NFC.


https://reviews.llvm.org/D50709



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

[PATCH] D50805: Don't warn on returning the address of a label

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7872-7874
-def warn_ret_addr_label : Warning<
-  "returning address of label, which is local">,
-  InGroup;

Why completely drop the diagnostic just because it is undesired in linux code?
Why not just add an `-Wreturn-stack-address` diag option instead, and disable 
it if undesired?


https://reviews.llvm.org/D50805



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

[PATCH] D50815: Establish the header

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/bit:99
+  unsigned long where;
+  // Search from LSB to MSB for first set bit.
+  // Returns zero if no set bit is found.

Pretty sure this is the other way around here.
```
// Search from MSB to LSB for first set bit.
```


https://reviews.llvm.org/D50815



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

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: llvm/include/llvm/Support/YAMLTraits.h:453
 
-inline bool isNumber(StringRef S) {
-  static const char OctalChars[] = "01234567";
-  if (S.startswith("0") &&
-  S.drop_front().find_first_not_of(OctalChars) == StringRef::npos)
-return true;
-
-  if (S.startswith("0o") &&
-  S.drop_front(2).find_first_not_of(OctalChars) == StringRef::npos)
-return true;
+inline bool isNumeric(StringRef S) {
+  if (S.empty())

Passing-by thought, feel free to ignore.

Changes like these are a **great** targets for fuzzers.
Don't just rewrite the implementation, but instead write a new [optimized] 
function,
and add a fuzzer that would feed both of these functions **the same** input,
and assert the equality of their outputs. (and that neither of them crashes).

Would preserve the infinitely more readable code version, too.


https://reviews.llvm.org/D50839



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

[PATCH] D50805: [Sema] Don't warn on returning the address of a label

[Roman Lebedev via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D50805#1202578, @Meinersbur wrote:

> In https://reviews.llvm.org/D50805#1201956, @Quuxplusone wrote:
>
> > If you added a new option `-Wret-addr-label` as suggested above (for a 
> > total patch of +2 lines), then is it accurate to say:
> >
> > - if `-Wret-addr-label` was enabled by default, we know of at least one 
> > codebase that would pass `-Wno-ret-addr-label` to their build
> > - if `-Wret-addr-label` was disabled by default, we don't know of any 
> > codebases that would voluntarily enable it And if nobody would enable it 
> > voluntarily... might as well eliminate it, right?
>
>
> That nobody here can name a project that would enable it, does not mean that 
> there is none, or that projects will decide in the future to use it, or that 
> individual developers temporarily use. Besides, it'd be enabled by 
> `-Weverything`.


+1 i **strongly** believe this should just be adding a new diag group, not 
completely dropping the diagnostic.


Repository:
  rC Clang

https://reviews.llvm.org/D50805



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

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: llvm/unittests/Support/YAMLIOTest.cpp:2626
+  //
+  // * Sexagecimal numbers
+  // * Decimal numbers with comma s the delimiter

Spelling



Comment at: llvm/unittests/Support/YAMLIOTest.cpp:2627
+  // * Sexagecimal numbers
+  // * Decimal numbers with comma s the delimiter
+  // * "inf", "nan" without '.' prefix

spelling


https://reviews.llvm.org/D50839



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

[PATCH] D50852: abseil-auto-make-unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

1. Please always upload all patches with full context.
2. There already is `modernize-use-auto`. Does it handle this case? Then this 
should be just an alias to that check. Else, i think it would be best to extend 
that one, and still alias.


https://reviews.llvm.org/D50852



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

[PATCH] D50852: abseil-auto-make-unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D50852#1202781, @hugoeg wrote:

> what do you mean by "with full context"?


`-U9` when generating the diff.


https://reviews.llvm.org/D50852



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

[PATCH] D50852: abseil-auto-make-unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added reviewers: aaron.ballman, zinovy.nis.
lebedev.ri added a comment.

In https://reviews.llvm.org/D50852#1203009, @hugoeg wrote:

> In https://reviews.llvm.org/D50852#1202774, @lebedev.ri wrote:
>
> > 1. Please always upload all patches with full context.
> > 2. There already is `modernize-use-auto`. Does it handle this case? Then 
> > this should be just an alias to that check. Else, i think it would be best 
> > to extend that one, and still alias.
>
>
> since this check checks for absl::make_unique primarily 
>  if we move it to the general check, it'd be weird to check for 
> absl::make_unique


Why do you think it would be weird?
That list should/would be a user-configurable config option anyway.

> right now our main goal is to release checks tailored specifically to abseil 
> users, so if we can we would like to release this check separately in the 
> abseil directory

Just checking, was there some memo i missed that abseil-related checks are 
exempt from all the usual guidelines?
Because this check is really not abseil-library-specific.


https://reviews.llvm.org/D50852



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

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D50616#1203751, @rjmccall wrote:

> In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:
>
> >
>
>
> Has anyone actually asked LLVM whether they would accept fixed-point types 
> into IR?  I'm just a frontend guy, but it seems to me that there are 
> advantages to directly representing these operations in a portable way even 
> if there are no in-tree targets providing special support for them.  And 
> there are certainly in-tree targets that could provide such support if 
> someone was motivated to do it.


Even just adding one new LLVM IR instruction (well, intrinsic too, ideally) 
already 'requires' you to to
then go around and make sure it is properly handled wrt all the other 
instructions, optimizations, codegen.

Adding a whole new type, i suspect, would be *much* more impactful.
And since it can already be represented via existing operations on existing 
integer type,
it isn't obvious why that would be the right way forward.


Repository:
  rC Clang

https://reviews.llvm.org/D50616



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

[PATCH] D50876: Clean up newly created header

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/bit:117
+  unsigned long __where;
   // Search from LSB to MSB for first set bit.
   // Returns zero if no set bit is found.

Like i commented in the original review, this should probably be 
```
// Search from *M*SB to *L*SB for first set bit.
```


https://reviews.llvm.org/D50876



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

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: rsmith, vsk, Sanitizers.
lebedev.ri added projects: Sanitizers, clang.

As per IRC disscussion, it seems we really want to have more fine-grained 
`-fsanitize=implicit-integer-truncation`:

- A check when both of the types are unsigned.
- Another check for the other cases (either one of the types is signed, or both 
of the types is signed).

This is clang part.
Compiler-rt part is .


Repository:
  rC Clang

https://reviews.llvm.org/D50901

Files:
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/catch-implicit-integer-conversions-basics.c
  test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c
  test/CodeGen/catch-implicit-integer-truncations-basics.c
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGen/catch-implicit-signed-integer-truncations-basics-negatives.c
  test/CodeGen/catch-implicit-signed-integer-truncations-basics.c
  test/CodeGen/catch-implicit-unsigned-integer-truncations-basics-negatives.c
  test/CodeGen/catch-implicit-unsigned-integer-truncations-basics.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,22 +29,37 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation),?){6}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){7}"}}
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
-// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implici

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri planned changes to this revision.
lebedev.ri added a comment.

Depends on https://reviews.llvm.org/D50901.
(which should land first, ideally.)


Repository:
  rC Clang

https://reviews.llvm.org/D50250



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

[PATCH] D50924: [CodeGen] add rotate builtins

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Looks ok to me.


https://reviews.llvm.org/D50924



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

[PATCH] D50852: [clang-tidy] abseil-auto-make-unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



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

zinovy.nis wrote:
> JonasToth wrote:
> > Please clang-format, `return` on next line.
> `auto` first appeared in C++11, so you may use `getLangOpts().CPlusPlus11`
> And `std::make_unique` is a part of C++14, so even `getLangOpts().CPlusPlus14.
I agree on the `auto` part, but it really shouldn't care any further than that.
You can write custom `yournamespace::MakeUnique<>` in C++11.
(As i have already wrote, the list if `MakeUnique` functions-to-be-searched 
should be a config option.)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50852



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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+  auto Filename = FileEntry->getName();
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");

hokein wrote:
> The regex seems incomplete, we are missing `algorithm`, `time` subdirectory. 
So what happens if open the namespace in a file that is located in my personal 
`absl/base` directory?
It will be false-negatively not reported?


https://reviews.llvm.org/D50580



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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+  auto Filename = FileEntry->getName();
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");

hokein wrote:
> lebedev.ri wrote:
> > hokein wrote:
> > > The regex seems incomplete, we are missing `algorithm`, `time` 
> > > subdirectory. 
> > So what happens if open the namespace in a file that is located in my 
> > personal `absl/base` directory?
> > It will be false-negatively not reported?
>  Yes, I'd say this is a known limitation.
Similarly, the check will break (start producing false-positives) as soon as a 
new directory is added to abseil proper.
I don't have any reliable idea on how to do this better, but the current 
solution seems rather suboptimal..


https://reviews.llvm.org/D50580



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

[PATCH] D50766: Fix false positive unsequenced access and modification warning in array subscript expression.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Test coverage?
Also, please always upload all patches with full context (`-U9`)


https://reviews.llvm.org/D50766



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

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c:23
+__attribute__((no_sanitize("integer"))) unsigned char 
blacklist_1_convert_unsigned_int_to_unsigned_char(unsigned int x) {
+  // CHECK: }
+  return x;

vsk wrote:
> It looks like 'CHECK: }' is meant to check that the end of the function is 
> reached without any diagnostic handlers being emitted. If so, you'll need 
> something stricter to actually check that.
> 
> Considering removing all of the 'CHECK: }' lines and adding 
> `-implicit-check-not=__ubsan_handle_implicit` as a FileCheck option.
I already specify `-implicit-check-not="call void 
@__ubsan_handle_implicit_conversion"`.
As for the `// CHECK: }`, i know it doesn't do anything by itself, so i could 
indeed drop that indeed.


Repository:
  rC Clang

https://reviews.llvm.org/D50901



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

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 162040.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Rebased, addressed @vsk review note, should be NFC.

@vsk thank you for taking a look!

>  LebedevRI: It looks like it's in good shape. I wasn't around for 
> the discussion re: splitting up the (un)signed cases, so it'd be good to have 
> someone involved with that sign off

@rsmith ?


Repository:
  rC Clang

https://reviews.llvm.org/D50901

Files:
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/catch-implicit-integer-conversions-basics.c
  test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c
  test/CodeGen/catch-implicit-integer-truncations-basics.c
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGen/catch-implicit-signed-integer-truncations-basics-negatives.c
  test/CodeGen/catch-implicit-signed-integer-truncations-basics.c
  test/CodeGen/catch-implicit-unsigned-integer-truncations-basics-negatives.c
  test/CodeGen/catch-implicit-unsigned-integer-truncations-basics.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,22 +29,37 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation),?){6}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){7}"}}
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
-// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 162050.
lebedev.ri added a subscriber: chandlerc.
lebedev.ri added a comment.

Rebased ontop of https://reviews.llvm.org/D50901, added

  -  ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit
 conversions that change the arithmetic value of the integer. Enables
 ``implicit-signed-integer-truncation`` and 
``implicit-integer-sign-change``.

as requested by @rsmith and @chandlerc.


Repository:
  rC Clang

https://reviews.llvm.org/D50250

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/catch-implicit-conversions-basics.c
  test/CodeGen/catch-implicit-integer-arithmetic-value-change-basics.c
  test/CodeGen/catch-implicit-integer-conversions-basics.c
  test/CodeGen/catch-implicit-integer-sign-changes-basics.c
  test/CodeGen/catch-implicit-integer-sign-changes-true-negatives.c
  test/CodeGen/catch-implicit-integer-sign-changes.c
  test/CodeGen/catch-implicit-signed-integer-truncation-or-sign-change.c
  test/CodeGenCXX/catch-implicit-integer-sign-changes-true-negatives.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,22 +29,52 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){7}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){8}"}}
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
-// CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}} // ???
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1050-1051
+// NOTE: if it is unsigned, then the comparison is naturally always 
'false'.
+llvm::ICmpInst::Predicate Pred =
+VSigned ? llvm::ICmpInst::ICMP_SLT : llvm::ICmpInst::ICMP_ULT;
+// Get the zero of the same type with which we will be comparing.

lebedev.ri wrote:
> rsmith wrote:
> > lebedev.ri wrote:
> > > rsmith wrote:
> > > > If `!VSigned`, the result is a constant `false`; you don't need to emit 
> > > > an `icmp` to work that out.
> > > Ok, if you insist.
> > > I didn't do that in the first place because we will now have an `icmp`
> > > where one operand being a constant, so we can simplify it further.
> > > And i don't want to complicate this logic if middle-end already handles 
> > > it :)
> > This becomes a lot simpler with the approach I described in the other 
> > comment thread, because you don't need a second `icmp eq` at all.
> Humm. So i have initially did this. It is probably broken for non-scalars, 
> but we don't care probably.
> 
> But then i thought more.
> 
> If we do not emit truncation check, we get `icmp eq (icmp ...), false`, which 
> is tautological.
> We can't just drop the outer `icmp eq` since we'd get [[ 
> https://rise4fun.com/Alive/4slv | the opposite value ]].
> We could emit `xor %icmp, -1` to invert it.  Or simply invert the predicate, 
> and avoid the second `icmp`.
> By itself, either of these options doesn't sound that bad.
> 
> But if both are signed, we can't do that. So we have to have two different 
> code paths...
> 
> If we do emit the `icmp ult %x, 0`, [it naturally works with vectors], we 
> avoid complicating the front-end,
> and the middle-end playfully simplifies this IR with no sweat.
> 
> So why do we want to complicate the front-end //in this case//, and not let 
> the middle-end do it's job?
> I'm unconvinced, and i have kept this as is. :/
> If !VSigned, the result is a constant false; you don't need to emit an icmp 
> to work that out.

Even at `-O0`, dagcombine constant-folds (unsurprizingly) this case, 
https://godbolt.org/z/D5ueOq


Repository:
  rC Clang

https://reviews.llvm.org/D50250



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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: alexfh, sbenza.
Herald added subscribers: mgrang, xazax.hun.

Continuation of https://reviews.llvm.org/D46504.

Example output:

  $ clang-tidy -enable-check-profile -store-check-profile=. 
-store-check-profile-elide-prefix=. -checks=-*,readability-function-size 
source.cpp 2>&1 >/dev/null
  $ cat .csv
  "User Time","System Time","User+System","Wall Time","Name"
  
8.842049973945e-01,1.20267757e-01,1.004472997370e+00,1.0031676292419434e+00,"readability-function-size"
  
8.842049973945e-01,1.20267757e-01,1.004472997370e+00,1.0031676292419434e+00,"Total"

There are two arguments that control profile storage:

- `-store-check-profile=` This option controls the prefix where these 
per-TU profiles are stored as CSV. If the prefix is not an absolute path, it is 
considered to be relative to the directory from where you have run 
`clang-tidy`. All `.` and `..` patterns in the path are collapsed, and symlinks 
are resolved.

  **Example**: Let's suppose you have a source file named `example.cpp`, 
located in `/source` directory.
  - If you specify `-store-check-profile=/tmp`, then the profile will be saved 
to `/tmp/source/example.cpp.csv`
  - If you run `clang-tidy` from within `/foo` directory, and specify 
`-store-check-profile=.`, then the profile will still be saved to 
`/foo/source/example.cpp.csv`
- `-store-check-profile-elide-prefix=` When specified, this prefix will 
be elided from the source file name, before prepending it with the prefix 
specified by `-store-check-profile`. If the prefix is not an absolute path, it 
is considered to be relative to the directory from where you have run 
`clang-tidy`. All `.` and `..` patterns in the path are collapsed, and symlinks 
are resolved.

  **Example**: Let's suppose you have a source file named `example.cpp`, 
located in `/source` directory.
  - If you specify `-store-check-profile=/tmp 
-store-check-profile-elide-prefix=/source` , then the profile will be saved to 
`/tmp/example.cpp.csv`
  - If you run `clang-tidy` from within `/source` directory, and specify 
`-store-check-profile=/foo -store-check-profile-elide-prefix=.`, then the 
profile will be saved to `/foo/example.cpp.csv`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,17 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.csv -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE: ===-===
+// CHECK-CONSOLE-NEXT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NEXT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NEXT: {{.*}}  Total
+// CHECK-CONSOLE-NEXT: ===-===
+
+// CHECK-FILE: {{.*}}"Wall Time","Name"
+// CHECK-FILE-NEXT: {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},"readability-function-size"
+// CHECK-FILE-NEXT: {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},"Total"
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -99,114 +99,127 @@
 
 .. code-block:: console
 
-  $ clang-tidy -help
+  $ clang-tidy --help
   USAGE: clang-tidy [options]  [... ]
 
   OPTIONS:
 
   Generic Options:
 
--help- Display available options (-help-hidden for more)
--help-list   - Display list of available options (-help-list-hidden for more)
--version - Display the version of this program
+-help  - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version   - Display the version of this program
 
   clang-tidy options:
 
--checks= -
-   Comma-separated list of globs with optional '-'
-   prefix. Globs 

[PATCH] D46603: [Support] Print TimeRecord as CSV

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: alexfh, sbenza, bkramer, george.karpenkov.

This is needed for the continuation of https://reviews.llvm.org/D46504,
to be able to store the timings as CSV.

The floating-point values are dumped with no precision loss.

See dependent differential https://reviews.llvm.org/D46602 for details/use-case.


Repository:
  rL LLVM

https://reviews.llvm.org/D46603

Files:
  include/llvm/Support/Timer.h
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -169,6 +171,35 @@
 OS << format("%9" PRId64 "  ", (int64_t)getMemUsed());
 }
 
+static void dumpVal(double Val, raw_ostream &OS) {
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  // FIXME: can ',' be a decimal separator?
+  OS << format("%.*e", max_digits10 - 1, Val);
+}
+
+void TimeRecord::printCSV(const TimeRecord &Total, raw_ostream &OS) const {
+  int Column = 0;
+  auto comma = [&Column, &OS]() {
+if (Column)
+  OS << ','; // FIXME: can ',' be a decimal separator?
+++Column;
+  };
+  auto printColumn = [comma, &OS](bool Print, double Val) {
+if (!Print)
+  return;
+comma();
+dumpVal(Val, OS);
+  };
+
+  printColumn(Total.getUserTime(), getUserTime());
+  printColumn(Total.getSystemTime(), getSystemTime());
+  printColumn(Total.getProcessTime(), getProcessTime());
+  printColumn(true, getWallTime());
+  if (Total.getMemUsed()) {
+comma();
+OS << format("%9" PRId64, (int64_t)getMemUsed());
+  }
+}
 
 
//===--===//
 //   NamedRegionTimer Implementation
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -64,6 +64,10 @@
   /// Print the current time record to \p OS, with a breakdown showing
   /// contributions to the \p Total time record.
   void print(const TimeRecord &Total, raw_ostream &OS) const;
+
+  /// Print the current time record to \p OS as CSV, with full precision.
+  /// Only the values in this timer are printed, no percentages.
+  void printCSV(const TimeRecord &Total, raw_ostream &OS) const;
 };
 
 /// This class is used to track the amount of time spent between invocations of


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -169,6 +171,35 @@
 OS << format("%9" PRId64 "  ", (int64_t)getMemUsed());
 }
 
+static void dumpVal(double Val, raw_ostream &OS) {
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  // FIXME: can ',' be a decimal separator?
+  OS << format("%.*e", max_digits10 - 1, Val);
+}
+
+void TimeRecord::printCSV(const TimeRecord &Total, raw_ostream &OS) const {
+  int Column = 0;
+  auto comma = [&Column, &OS]() {
+if (Column)
+  OS << ','; // FIXME: can ',' be a decimal separator?
+++Column;
+  };
+  auto printColumn = [comma, &OS](bool Print, double Val) {
+if (!Print)
+  return;
+comma();
+dumpVal(Val, OS);
+  };
+
+  printColumn(Total.getUserTime(), getUserTime());
+  printColumn(Total.getSystemTime(), getSystemTime());
+  printColumn(Total.getProcessTime(), getProcessTime());
+  printColumn(true, getWallTime());
+  if (Total.getMemUsed()) {
+comma();
+OS << format("%9" PRId64, (int64_t)getMemUsed());
+  }
+}
 
 //===--===//
 //   NamedRegionTimer Implementation
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -64,6 +64,10 @@
   /// Print the current time record to \p OS, with a breakdown showing
   /// contributions to the \p Total time record.
   void print(const TimeRecord &Total, raw_ostream &OS) const;
+
+  /// Print the current time record to \p OS as CSV, with full precision.
+  /// Only the values in this timer are printed, no percentages.
+  void printCSV(const TimeRecord &Total, raw_ostream &OS) const;
 };
 
 /// This class is used to track the amount of time spent between invocations of
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[Diffusion] rL331456: [clang-tidy] Remove AnalyzeTemporaryDtors option.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: cfe-commits, alexfh, lebedev.ri.
lebedev.ri raised a concern with this commit.
lebedev.ri added a comment.

Every single `.clang-tidy` config file that still happens to contain
`AnalyzeTemporaryDtors: true/false` param specified is now **silently** (!) 
ignored,
and a default config is used. I have found that out the hard way.


Users:
  alexfh (Author)
  lebedev.ri (Auditor)

https://reviews.llvm.org/rL331456



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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D46602#1092084, @Eugene.Zelenko wrote:

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


Yeah, i have thought about it, and i'm not sure.
The output is so dumb so there isn't even much point in using anything more 
advanced than CSV.


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] D46603: [Support] Print TimeRecord as CSV

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D46603#1092135, @george.karpenkov wrote:

> @lebedev.ri LLVM already has facilities for outputting statistics in JSON, it 
> seems that would be sufficient for your purposes?
>  I did something similar for the static analyzer in 
> https://reviews.llvm.org/D43131


YAML != JSON.
I did see all that `TimerGroup` stuff.
I will take a second look, but the `dumpVal()` change will still be needed 
either way.


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] D46602: [clang-tidy] Store checks profiling info as CSV files

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri planned changes to this revision.
lebedev.ri added inline comments.



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

Eugene.Zelenko wrote:
> Please use ` for command line options and other non-C/C++ constructs.
`` is used for command line options elsewhere in this document.
Is there a reference somewhere?


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] D46603: [Support] TimerGroup changes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 145900.
lebedev.ri retitled this revision from "[Support] Print TimeRecord as CSV" to 
"[Support] TimerGroup changes".
lebedev.ri edited the summary of this revision.
lebedev.ri added a reviewer: NoQ.
lebedev.ri added subscribers: xazax.hun, szepet, a.sidorin.
lebedev.ri added a comment.

1. Drop CSV stuff
2. Add constructor from previously-collected `StringMap`
3. Make `printJSONValues()` public.
4. Dump floating-point values in scientific format with full precision.


Repository:
  rL LLVM

https://reviews.llvm.org/D46603

Files:
  include/llvm/Support/Timer.h
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -234,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
@@ -367,10 +378,12 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
- "Timer name needs no quotes");
-  OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
+ "Timer name should not need quotes");
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  OS << "\t\"time." << Name << '.' << R.Name << suffix
+ << "\": " << format("%.*e", max_digits10 - 1, Value);
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -10,6 +10,7 @@
 #ifndef LLVM_SUPPORT_TIMER_H
 #define LLVM_SUPPORT_TIMER_H
 
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include 
@@ -194,6 +195,10 @@
 
 public:
   explicit TimerGroup(StringRef Name, StringRef Description);
+
+  explicit TimerGroup(StringRef Name, StringRef Description,
+  const StringMap &Records);
+
   ~TimerGroup();
 
   void setName(StringRef NewName, StringRef NewDescription) {
@@ -207,6 +212,8 @@
   /// This static method prints all timers and clears them all out.
   static void printAll(raw_ostream &OS);
 
+  const char *printJSONValues(raw_ostream &OS, const char *delim);
+
   /// Prints all timers as JSON key/value pairs, and clears them all out.
   static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 
@@ -223,7 +230,6 @@
   void PrintQueuedTimers(raw_ostream &OS);
   void printJSONValue(raw_ostream &OS, const PrintRecord &R,
   const char *suffix, double Value);
-  const char *printJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -234,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
@@ -367,10 +378,12 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 145901.
lebedev.ri retitled this revision from "[clang-tidy] Store checks profiling 
info as CSV files" to "[clang-tidy] Store checks profiling info as YAML files".
lebedev.ri added reviewers: george.karpenkov, NoQ.
lebedev.ri added a comment.

- Deduplicate code by switching to already-existing YAML printer infrastructure 
from `TimerGroup`
- Switch to YAML. It's kinda ugly, but maybe better than having to manually 
construct CSV.. :/
- When the output prefix does not yet exist, still store the profile as YAML, 
don't fail at `make_real`
- When storing profile to file, don't print it to screen. I have tried it, and 
it is just too noisy for no apparent benefit.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
  test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,17 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.yaml -check-prefix=CHECK-FILE %s
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/out/.yaml -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}}  Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK-NOT: ===-===
+// CHECK-NOT:  clang-tidy checks profiling
+// CHECK-NOT: ===-===
+// CHECK-NOT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
 // CHECK-NOT: {{.*}}  --- Name ---
 // CHECK-NOT: {{.*}}  readability-function-size
 // CHECK-NOT: {{.*}}  Total
-// CHECK-NOT: ===-===
 
 class A {
   A() {}
Index: test/clang-tidy/clang-tidy-enable-check-prof

[PATCH] D46603: [Support] TimerGroup changes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 145903.
lebedev.ri added a comment.

Add lock to now-public `printJSONValues()`.

I have no understanding of the `static TimerGroupList`,
but this is symmetrical with other [public] `TimerGroup::*print*` methods.


Repository:
  rL LLVM

https://reviews.llvm.org/D46603

Files:
  include/llvm/Support/Timer.h
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -234,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
@@ -367,13 +378,17 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
- "Timer name needs no quotes");
-  OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
+ "Timer name should not need quotes");
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  OS << "\t\"time." << Name << '.' << R.Name << suffix
+ << "\": " << format("%.*e", max_digits10 - 1, Value);
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
+  sys::SmartScopedLock L(*TimerLock);
+
   prepareToPrintList();
   for (const PrintRecord &R : TimersToPrint) {
 OS << delim;
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -10,6 +10,7 @@
 #ifndef LLVM_SUPPORT_TIMER_H
 #define LLVM_SUPPORT_TIMER_H
 
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include 
@@ -194,6 +195,10 @@
 
 public:
   explicit TimerGroup(StringRef Name, StringRef Description);
+
+  explicit TimerGroup(StringRef Name, StringRef Description,
+  const StringMap &Records);
+
   ~TimerGroup();
 
   void setName(StringRef NewName, StringRef NewDescription) {
@@ -207,6 +212,8 @@
   /// This static method prints all timers and clears them all out.
   static void printAll(raw_ostream &OS);
 
+  const char *printJSONValues(raw_ostream &OS, const char *delim);
+
   /// Prints all timers as JSON key/value pairs, and clears them all out.
   static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 
@@ -223,7 +230,6 @@
   void PrintQueuedTimers(raw_ostream &OS);
   void printJSONValue(raw_ostream &OS, const PrintRecord &R,
   const char *suffix, double Value);
-  const char *printJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -234,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
@@ -367,13 +378,17 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
- "Timer name needs no quotes");
-  OS << "

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 145906.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.
Herald added a subscriber: llvm-commits.

- Produce valid-er YAML.


Repository:
  rL LLVM

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
  test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,23 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.yaml -check-prefix=CHECK-FILE %s
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/out/.yaml -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}}  Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE: }
+
+// CHECK-NOT: {
+// CHECK-NOT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-NOT: }
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK-NOT: ===-===
+// CHECK-NOT:  clang-tidy checks profiling
+// CHECK-NOT: ===-===
+// CHECK-NOT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
 // CHECK-NOT: {{.*}}  --- Name ---
 // CHECK-NOT: {{.*}}  readability-function-size
 // CHECK-NOT: {{.*}}  Total
-// CHECK-NOT: ===-===
 
 class A {
   A() {}
Index: test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
@@ -1,16 +1,22 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s 2>&1 | FileCheck --match-fu

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: docs/clang-tidy/index.rst:783
+  {
+"time.clang-tidy.readability-function-size.wall": 1.0421266555786133e+00,
+"time.clang-tidy.readability-function-size.user": 9.208840005421e-01,

Hmm, thinking about it a bit more, i think it should also contain the `"file"`,
```
{
  "file": "/source/example.cpp",
  "profile": {
"time.clang-tidy.readability-function-size.wall": 9.5367431640625000e-05,
"time.clang-tidy.readability-function-size.user": 7.20002617e-05,
"time.clang-tidy.readability-function-size.sys": 1.8920e-05
  }
}
```


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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D46602#1092883, @alexfh wrote:

> 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?


I personally don't have a preference here.
The "YAML" may be better because that is what already supported by 
`TimerGroup`, which allowed to drop the table printer, too.

The next step will be a python script:

1. Takes either a file names (these generated `.yaml`), or a prefix, If it is a 
prefix, it GLOBS all the `.yaml`'s in there.
2. Load all the files from step 1.
3. Print global report from data collected (`reduce(+)` after grouping by check 
name) from all the files (what https://reviews.llvm.org/D45931 did)
4. Maybe print report about outliers - without "`reduce(+)` after grouping by 
check name", with percentages to the total time spent on TU. This is where the 
filename (and YAML) would be helpful.
5. ???
6. Run on codebases, find performance problems, contribute fixes
7. Profit!


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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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?

I don't like this, because when working on trying to improve the performance of 
the check,
one wouldn't touch the source file, only the clang-tidy sources. So either you 
would 
have to `touch` sources (and if they are not writable?), or remove `.csv` 
beforehand,
or not output to file, but redirect output.

Neither of these possibilities sound great to me.

> 2. include just the name of the file without any parent directories,

That won't work, there are duplicate filenames even in LLVM.

  $ 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 :)


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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri 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?
>
> I don't like this, because when working on trying to improve the performance 
> of the check,
>  one wouldn't touch the source file, only the clang-tidy sources. So either 
> you would 
>  have to `touch` sources (and if they are not writable?), or remove `.csv` 
> beforehand,
>  or not output to file, but redirect output.


... also, a new report with a new name will be created each time the filetime 
changes, so not
only will it be fun from tooling point of view, but it will also leave old 
reports in-place..

> Neither of these possibilities sound great to me.
> 
>> 2. include just the name of the file without any parent directories,
> 
> That won't work, there are duplicate filenames even in LLVM.
> 
>   $ 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 :)




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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 145994.
lebedev.ri added a comment.

- Use sane (not the same as the one right before..) suffix for when 
json-printing mem usage. Admittedly, i haven't tried it, but it just does not 
make sense otherwise.


Repository:
  rL LLVM

https://reviews.llvm.org/D46603

Files:
  include/llvm/Support/Timer.h
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -234,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
@@ -367,13 +378,17 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
- "Timer name needs no quotes");
-  OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
+ "Timer name should not need quotes");
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  OS << "\t\"time." << Name << '.' << R.Name << suffix
+ << "\": " << format("%.*e", max_digits10 - 1, Value);
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
+  sys::SmartScopedLock L(*TimerLock);
+
   prepareToPrintList();
   for (const PrintRecord &R : TimersToPrint) {
 OS << delim;
@@ -387,7 +402,7 @@
 printJSONValue(OS, R, ".sys", T.getSystemTime());
 if (T.getMemUsed()) {
   OS << delim;
-  printJSONValue(OS, R, ".sys", T.getMemUsed());
+  printJSONValue(OS, R, ".mem", T.getMemUsed());
 }
   }
   TimersToPrint.clear();
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -10,6 +10,7 @@
 #ifndef LLVM_SUPPORT_TIMER_H
 #define LLVM_SUPPORT_TIMER_H
 
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include 
@@ -194,6 +195,10 @@
 
 public:
   explicit TimerGroup(StringRef Name, StringRef Description);
+
+  explicit TimerGroup(StringRef Name, StringRef Description,
+  const StringMap &Records);
+
   ~TimerGroup();
 
   void setName(StringRef NewName, StringRef NewDescription) {
@@ -207,6 +212,8 @@
   /// This static method prints all timers and clears them all out.
   static void printAll(raw_ostream &OS);
 
+  const char *printJSONValues(raw_ostream &OS, const char *delim);
+
   /// Prints all timers as JSON key/value pairs, and clears them all out.
   static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 
@@ -223,7 +230,6 @@
   void PrintQueuedTimers(raw_ostream &OS);
   void printJSONValue(raw_ostream &OS, const PrintRecord &R,
   const char *suffix, double Value);
-  const char *printJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -234,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
@@ -367,13 +378,17 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::ne

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 145995.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.

- Make json less flat, store source filename in it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
  test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,33 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.yaml -check-prefix=CHECK-FILE %s
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/out/.yaml -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}}  Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK-NOT: ===-===
+// CHECK-NOT:  clang-tidy checks profilin

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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?

>>> 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] D45835: Add new driver mode for dumping compiler options

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Frontend/FrontendActions.cpp:779
+  {
+std::string Str;
+#define FEATURE(Name, Predicate)   
\

This is likely to do an allocation for each feature.
Maybe consider `llvm::SmallString<64>`



Comment at: lib/Frontend/FrontendActions.cpp:781
+#define FEATURE(Name, Predicate)   
\
+  Str += "\t{\"" #Name "\" : " + std::string(Predicate ? "true" : "false") +   
\
+ "},\n";

Similarly, you may want to explicitly use `llvm::Twine` here



Comment at: lib/Frontend/FrontendActions.cpp:791
+
+  OS << "\n\"extensions\" : [\n";
+  {

same


https://reviews.llvm.org/D45835



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

[PATCH] D46805: If some platforms do not support an attribute, we should exclude the platform

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

1. Please always upload all patches with full context.
2. tests?


Repository:
  rC Clang

https://reviews.llvm.org/D46805



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Frontend/FrontendActions.cpp:779
+  {
+std::string Str;
+#define FEATURE(Name, Predicate)   
\

aaron.ballman wrote:
> lebedev.ri wrote:
> > This is likely to do an allocation for each feature.
> > Maybe consider `llvm::SmallString<64>`
> 64 bytes seemed a bit tight, so I went with 128 instead. Likely still a bit 
> too small, but shouldn't be too bad.
`FixedString` / `FixedVector` would be nice to have here.


https://reviews.llvm.org/D45835



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

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45766#1097736, @ksu.shadura wrote:

> Hi,


Hi.

> we see the false-positive behavior of -Wno-self-assign-overloaded flag in 
> case of subtraction assignment operator. 
>  The minimal reproducer that we got: https://godbolt.org/g/8PQMpR
>  ...

Could you please clarify, what is the false-positive?
The fact that it fires on overloaded `-=` is intended behavior, and is tested 
in tests, see `cfe/trunk/test/SemaCXX/warn-self-assign-overloaded.cpp`:

  #ifndef DUMMY
a *= a;
a /= a; // expected-warning {{explicitly assigning}}
a %= a; // expected-warning {{explicitly assigning}}
a += a;
a -= a; // expected-warning {{explicitly assigning}}
a <<= a;
a >>= a;
a &= a; // expected-warning {{explicitly assigning}}
a |= a; // expected-warning {{explicitly assigning}}
a ^= a; // expected-warning {{explicitly assigning}}
  #endif



> Thanks in advance!




Repository:
  rL LLVM

https://reviews.llvm.org/D45766



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

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45766#1097797, @ksu.shadura wrote:

> Thank you for the test example! I got your point, but I wanted to ask if it 
> should be like this for commutative operations?




> In our case it is actually matrix, and subtraction of matrices is not 
> commutative operation..

With that context, i do agree that it is a false-positive.
Please file a bug (do we already have a 7.0 metabug yet?).

I'm not too sure what is the best way to solve this.
The most obvious solution would be taking the operator status into account,
like it was considered during review, https://reviews.llvm.org/D44883#1048520, 
https://godbolt.org/g/7K7Uwy - and if it is "non_trivial",
then issue it under `-Wself-assign-overloaded-nontrivial` ?


Repository:
  rL LLVM

https://reviews.llvm.org/D45766



___
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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.

> 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] D46602: [clang-tidy] Store checks profiling info as YAML files

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 146644.
lebedev.ri edited the summary of this revision.
lebedev.ri added subscribers: aaron.ballman, JonasToth.
lebedev.ri added a comment.

- Get rid of 'prefix elision'
- Don't use full source file name, only the filename
- Prefix that filename with ISO8601-like timestamp of the profile creation.

I think this is worse than before, horrible and ugly, but hey, i'm not the code 
owner.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
  test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,35 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}}  Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readabili

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a project: clang-tools-extra.
lebedev.ri added a comment.

In https://reviews.llvm.org/D46602#1098092, @alexfh wrote:

> In https://reviews.llvm.org/D46602#1097954, @lebedev.ri wrote:
>
> > How do i reflect that in tests? The output name will basically be random.
>
>
> Why random?


I was obviously talking about the FileCheck stuff, how to specify such a 
dynamic filename there..


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] D46603: [Support] TimerGroup changes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ping.
Any thoughts on these Timer changes?


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] D46927: Augmented Assignment for Fixed Point Types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Hi @leonardchan.
Could you please prepend some appropriate tag to the subjects of all your 
differentials, so it is less hard to read through cfe-commits mails, please?


Repository:
  rC Clang

https://reviews.llvm.org/D46927



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

[PATCH] D46937: [Timers] TimerGroup::printJSONValue(): print doubles with no precision loss

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: george.karpenkov, NoQ, alexfh, sbenza.
Herald added a subscriber: llvm-commits.
lebedev.ri added a dependent revision: D46938: [Timers] TimerGroup: make 
printJSONValues() method public.
lebedev.ri added a dependency: D46936: [Timers] TimerGroup::printJSONValues(): 
print mem timer with .mem suffix.

Although this is not stricly required, i would very much prefer
not to have known random precision losses along the way.


Repository:
  rL LLVM

https://reviews.llvm.org/D46937

Files:
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -367,10 +369,12 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
- "Timer name needs no quotes");
-  OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
+ "Timer name should not need quotes");
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  OS << "\t\"time." << Name << '.' << R.Name << suffix
+ << "\": " << format("%.*e", max_digits10 - 1, Value);
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -22,6 +22,8 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -367,10 +369,12 @@
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
- "TimerGroup name needs no quotes");
+ "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
- "Timer name needs no quotes");
-  OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
+ "Timer name should not need quotes");
+  constexpr auto max_digits10 = std::numeric_limits::max_digits10;
+  OS << "\t\"time." << Name << '.' << R.Name << suffix
+ << "\": " << format("%.*e", max_digits10 - 1, Value);
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
___
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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: george.karpenkov, NoQ, alexfh, sbenza.
Herald added a subscriber: llvm-commits.
lebedev.ri added a dependent revision: D46937: [Timers] 
TimerGroup::printJSONValue(): print doubles with no precision loss.

We have just used `.sys` suffix for the previous timer, this is clearly a typo


Repository:
  rL LLVM

https://reviews.llvm.org/D46936

Files:
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -387,7 +387,7 @@
 printJSONValue(OS, R, ".sys", T.getSystemTime());
 if (T.getMemUsed()) {
   OS << delim;
-  printJSONValue(OS, R, ".sys", T.getMemUsed());
+  printJSONValue(OS, R, ".mem", T.getMemUsed());
 }
   }
   TimersToPrint.clear();


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -387,7 +387,7 @@
 printJSONValue(OS, R, ".sys", T.getSystemTime());
 if (T.getMemUsed()) {
   OS << delim;
-  printJSONValue(OS, R, ".sys", T.getMemUsed());
+  printJSONValue(OS, R, ".mem", T.getMemUsed());
 }
   }
   TimersToPrint.clear();
___
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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: george.karpenkov, NoQ, alexfh, sbenza.
Herald added a subscriber: llvm-commits.
lebedev.ri added a dependency: D46938: [Timers] TimerGroup: make 
printJSONValues() method public.
lebedev.ri added a dependent revision: D46602: [clang-tidy] Store checks 
profiling info as YAML files.

This is needed for the continuation of https://reviews.llvm.org/D46504,
to be able to store the timings.


Repository:
  rL LLVM

https://reviews.llvm.org/D46939

Files:
  include/llvm/Support/Timer.h
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -236,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -10,6 +10,7 @@
 #ifndef LLVM_SUPPORT_TIMER_H
 #define LLVM_SUPPORT_TIMER_H
 
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include 
@@ -194,6 +195,10 @@
 
 public:
   explicit TimerGroup(StringRef Name, StringRef Description);
+
+  explicit TimerGroup(StringRef Name, StringRef Description,
+  const StringMap &Records);
+
   ~TimerGroup();
 
   void setName(StringRef NewName, StringRef NewDescription) {


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -236,6 +236,15 @@
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+   const StringMap &Records)
+: TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -10,6 +10,7 @@
 #ifndef LLVM_SUPPORT_TIMER_H
 #define LLVM_SUPPORT_TIMER_H
 
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include 
@@ -194,6 +195,10 @@
 
 public:
   explicit TimerGroup(StringRef Name, StringRef Description);
+
+  explicit TimerGroup(StringRef Name, StringRef Description,
+  const StringMap &Records);
+
   ~TimerGroup();
 
   void setName(StringRef NewName, StringRef NewDescription) {
___
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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: george.karpenkov, NoQ, alexfh, sbenza.
Herald added a subscriber: llvm-commits.
lebedev.ri added a dependent revision: D46939: [Timers] TimerGroup: add 
constructor from StringMap.
lebedev.ri added a dependency: D46937: [Timers] TimerGroup::printJSONValue(): 
print doubles with no precision loss.

This is needed for the continuation of https://reviews.llvm.org/D46504,
to be able to store the timings.


Repository:
  rL LLVM

https://reviews.llvm.org/D46938

Files:
  include/llvm/Support/Timer.h
  lib/Support/Timer.cpp


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -378,6 +378,8 @@
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
+  sys::SmartScopedLock L(*TimerLock);
+
   prepareToPrintList();
   for (const PrintRecord &R : TimersToPrint) {
 OS << delim;
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -207,6 +207,8 @@
   /// This static method prints all timers and clears them all out.
   static void printAll(raw_ostream &OS);
 
+  const char *printJSONValues(raw_ostream &OS, const char *delim);
+
   /// Prints all timers as JSON key/value pairs, and clears them all out.
   static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 
@@ -223,7 +225,6 @@
   void PrintQueuedTimers(raw_ostream &OS);
   void printJSONValue(raw_ostream &OS, const PrintRecord &R,
   const char *suffix, double Value);
-  const char *printJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm


Index: lib/Support/Timer.cpp
===
--- lib/Support/Timer.cpp
+++ lib/Support/Timer.cpp
@@ -378,6 +378,8 @@
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
+  sys::SmartScopedLock L(*TimerLock);
+
   prepareToPrintList();
   for (const PrintRecord &R : TimersToPrint) {
 OS << delim;
Index: include/llvm/Support/Timer.h
===
--- include/llvm/Support/Timer.h
+++ include/llvm/Support/Timer.h
@@ -207,6 +207,8 @@
   /// This static method prints all timers and clears them all out.
   static void printAll(raw_ostream &OS);
 
+  const char *printJSONValues(raw_ostream &OS, const char *delim);
+
   /// Prints all timers as JSON key/value pairs, and clears them all out.
   static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 
@@ -223,7 +225,6 @@
   void PrintQueuedTimers(raw_ostream &OS);
   void printJSONValue(raw_ostream &OS, const PrintRecord &R,
   const char *suffix, double Value);
-  const char *printJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46603: [Support] TimerGroup changes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri abandoned this revision.
lebedev.ri added a comment.

In https://reviews.llvm.org/D46603#1100455, @george.karpenkov wrote:

> I see four separate changes: s/.sys/mem one (can be committed without 
> review), exposing printJSONValues and consequent adding of a lock, adding a 
> constructor accepting a map, and fixing formatting in `printJSONValue`. All 
> four look good to me, but probably should be reviewed separately.


Thank you for taking a look!
Posted as https://reviews.llvm.org/D46936, https://reviews.llvm.org/D46937, 
https://reviews.llvm.org/D46938, https://reviews.llvm.org/D46939


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] D46602: [clang-tidy] Store checks profiling info as YAML files

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 147071.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.

- Rebased
- Slightly refactor how the profile storage params are computed, move that into 
`ClangTidyProfiling::StorageParams` helper struct.
- Store timestamp in the json too, since it's already in the filename. May be 
useful for garbage collection and alike.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
  test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,37 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}}  Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45766#1101533, @ksu.shadura wrote:

> Thanks for your opinion, your guess was right! It is a unit-test, so seems we 
> need to try to suppress warnings on our side.


Ok, great that this time it got resolved this trivially!

> In https://reviews.llvm.org/D45766#1098823, @Quuxplusone wrote:
> 
>> (That said, I continue to think that this diagnostic produces more noise 
>> than signal, and wish it weren't in `-Wall`...)

Too bad there is no way to track how many true-positives it produces.
Anecdotal evidence: it just prevented me from wasting time trying to understand 
what is going on during https://reviews.llvm.org/D46602 development, so sure, 
it can have false-positive noise, but..


Repository:
  rL LLVM

https://reviews.llvm.org/D45766



___
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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 147120.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Rename getter/setter functions.

Thank you for taking a look!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46602

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyProfiling.cpp
  clang-tidy/ClangTidyProfiling.h
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
  test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
  test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp

Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,37 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}}  --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}}  readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}}  Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT:	"time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+  A() {}
+  ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
 // RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 // CHECK-NEXT: {{.*}}  Total
-// CHECK-NEXT: ===-===
 
 // CHECK: ===-===
-// CHECK-NEXT: {{.*}}  --- Name ---
+// CHECK-NEXT:  clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}}  --- Name ---
 // CHECK-NEXT: {{.*}}  readability-function-size
 /

[PATCH] D46603: [Support] TimerGroup changes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D46603#1101047, @lebedev.ri wrote:

> In https://reviews.llvm.org/D46603#1100455, @george.karpenkov wrote:
>
> > I see four separate changes: s/.sys/mem one (can be committed without 
> > review), exposing printJSONValues and consequent adding of a lock, adding a 
> > constructor accepting a map, and fixing formatting in `printJSONValue`. All 
> > four look good to me, but probably should be reviewed separately.
>
>
> Thank you for taking a look!
>  Posted as https://reviews.llvm.org/D46936, https://reviews.llvm.org/D46937, 
> https://reviews.llvm.org/D46938, https://reviews.llvm.org/D46939


@alexfh & @george.karpenkov Thanks for reviewing those!
Not sure yet whether i will land them right away, or wait for clang-tidy part.


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] D46602: [clang-tidy] Store checks profiling info as YAML files

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/tool/ClangTidyMain.cpp:188
+format to stderr. When this option is passed,
+these per-TU profiles are instead stored as YAML.)"),
+  cl::value_desc("prefix"),

Quuxplusone wrote:
> Drive-by nit: Each other help string ends with a newline (`)"),` on a line by 
> itself). This one presumably should too.
Hmm, yeah, at least for consistency.


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] D47191: [clang-format] Fix crash in getLengthToMatchingParen

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

No test?


Repository:
  rL LLVM

https://reviews.llvm.org/D47191



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

[PATCH] D47202: [CodeGen] use nsw negation for abs

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

That is what happens for "hand-rolled" `abs`, https://godbolt.org/g/6dbgXD
I *think* this makes sense, since **IIRC** LLVM only supports twos-complement 
platforms.
But all these attributes are currently beyond me, so i won't click the accept 
button :)


https://reviews.llvm.org/D47202



___
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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ping.


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] D45686: [Tooling] Clean up tmp files when creating a fixed compilation database

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added reviewers: aaron.ballman, john.brawn, mehdi_amini, sammccall, 
zturner, bkramer.
lebedev.ri added a comment.

The testcase sounds reasonable. Can't comment on the actual fix.
Adding some more reviewers that potentially worked on that source file.


Repository:
  rC Clang

https://reviews.llvm.org/D45686



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

[PATCH] D45686: [Tooling] Clean up tmp files when creating a fixed compilation database

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Tooling/CompilationDatabase.cpp:303
+  // Remove temp files.
+  Compilation->CleanupFileList(Compilation->getTempFiles());
+

dstenb wrote:
> erichkeane wrote:
> > It seems to me that this would be best called from the destructor of the 
> > Compilation object.  I realize nothing returns before this, but IMO this 
> > seems universal enough that if it doesn't break anything, putting this in 
> > the destructor would be a proper place for cleanup.
> That sounds cleaner (if it's viable).
> 
> I considered that initially, but I was not sure that there are no cases where 
> the temporary files outlive a Compilation object, and the "/// Temporary 
> files which should be removed on exit." comment for the TempFiles field did 
> not really make me more confident, so I went the conservative route instead.
> 
> I'll do that change and run some tests to see where that leads.
> 
It just looks off-place for the function named `stripPositionalArgs()`


Repository:
  rC Clang

https://reviews.llvm.org/D45686



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

[PATCH] D46188: [clang-tidy] Add a flag to enable debug checkers

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri abandoned this revision.
lebedev.ri added a comment.

https://reviews.llvm.org/D46187#1098571
Sad, but ok.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46188



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

[PATCH] D46187: [Analyzer] getRegisteredCheckers(): handle debug checkers too.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri abandoned this revision.
lebedev.ri added a comment.

Sad, but ok.


Repository:
  rC Clang

https://reviews.llvm.org/D46187



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/Driver/Compilation.h:126
+  /// Whether to save temporary files.
+  bool SaveTempsEnabled;
+

`KeepTempFiles`?



Comment at: lib/Driver/Compilation.cpp:276-277
+
+  // Temporary files added by diagnostics should be kept.
+  SaveTempsEnabled = true;
 }

Is there a test that breaks without this?


https://reviews.llvm.org/D45686



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

[PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please improve test coverage a bit.
`-Wunused-using` is part of `-Wunused`.
And `-Wunused` is part of `-Wall`.
That needs to be tested.

Also, one small test to show that it is not on by default,
and `-Wno-unused-using` actually disables it is needed.




Comment at: docs/ReleaseNotes.rst:117
+
+  -Wall now includes the new warning flag `-Wunused-using`, which emits a
+  warning on using statements that are not used. This may result in new

```
``-Wall``

```



Comment at: docs/ReleaseNotes.rst:117
+
+  -Wall now includes the new warning flag `-Wunused-using`, which emits a
+  warning on using statements that are not used. This may result in new

lebedev.ri wrote:
> ```
> ``-Wall``
> 
> ```
```
``-Wunused-using``
```



Comment at: include/clang/Basic/DiagnosticGroups.td:828-829
 // -Wunused-local-typedefs = -Wunused-local-typedef
+def : DiagGroup<"unused-usings", [UnusedUsing]>;
+// -Wunused-usings = -Wunused-using
 

Why? gcc compatibility?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:282-284
+def warn_unused_using : Warning<
+  "unused using %0">,
+  InGroup, DefaultIgnore;

http://en.cppreference.com/w/cpp/keyword/using says that there are multiple 
flavors.
It may be more friendlier to have more than one single message for all of them. 



Comment at: test/SemaCXX/referenced_alias_declaration_1.cpp:1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunused-using -verify %s
+

The svn attribute changes are likely unintentional.



Comment at: test/SemaCXX/referenced_alias_declaration_2.cpp:1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunused-using -verify %s
+

The svn attribute changes are likely unintentional.



Comment at: test/SemaCXX/referenced_using_all.cpp:1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunused-using -verify %s
+

The svn attribute changes are likely unintentional.



Comment at: test/SemaCXX/referenced_using_declaration_1.cpp:1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunused-using -verify %s
+

The svn attribute changes are likely unintentional.



Comment at: test/SemaCXX/referenced_using_declaration_2.cpp:1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunused-using -verify %s
+

The svn attribute changes are likely unintentional.



Comment at: test/SemaCXX/referenced_using_directive.cpp:1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunused-using -verify %s
+

The svn attribute changes are likely unintentional.


https://reviews.llvm.org/D44826



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

LGTM, but i'm quite unfamiliar with this area of the code,
so please wait for someone else to accept :)




Comment at: lib/Driver/Compilation.cpp:276-277
+
+  // Temporary files added by diagnostics should be kept.
+  SaveTempsEnabled = true;
 }

dstenb wrote:
> lebedev.ri wrote:
> > Is there a test that breaks without this?
> Yes, the following tests fail:
> 
>   - Driver/crash-report-modules.m
>   - Driver/crash-report-spaces.c
>   - Driver/crash-report-header.h
>   - Driver/crash-report.c
>   - Modules/crash-vfs-path-emptydir-entries.m
>   - Modules/crash-vfs-path-symlink-topheader.m
>   - Modules/crash-vfs-path-symlink-component.m
>   - Modules/crash-vfs-path-traversal.m
>   - Modules/crash-vfs-relative-overlay.m
>   - Modules/crash-vfs-umbrella-frameworks.m
> 
> due to the preprocessed files for the crash diagnostics having been removed.
Ok, good.


https://reviews.llvm.org/D45686



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45686#1113425, @Ka-Ka wrote:

> In https://reviews.llvm.org/D45686#1113414, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D45686#1109295, @dstenb wrote:
> >
> > > Ping.
> > >
> > > We have added a lit reproducer for this now in clang-tools-extra: 
> > > https://reviews.llvm.org/D47251.
> >
> >
> > The above should be rolled into this patch as the test case verifying the 
> > behavioral changes.
>
>
> I guess the intention from @dstenb was to submit both patches together. The 
> reason for a separate patch with the testcase alone is that the reproducer 
> use clang-tidy and as far as I know all clang-tidy tests exist in 
> clang-tools-extra.


.. and both of them exist in the **same** svn repo, or git monorepo. (granted, 
not many use that)


https://reviews.llvm.org/D45686



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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D38171#880016, @xazax.hun wrote:

> In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote:
>
> > 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, 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.
>
>
> That is somewhat intentional I think. Lets imagined you are interested in 
> cppcoreguidelines but not interested in high integrity cpp. It would be great 
> to be able to use Checks: `'cppcoreguidelines*,-hicpp*'` without having to 
> worry about the aliases.
>
> In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote:
>
> > 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.
>
>
> I think the source of the problems is that conceptually there are two 
> distinct reasons to disable a check. One reason when the user is not 
> interested in the results regardless what is the category or name of the 
> check. In this case, all aliases should be disabled. The second is when a 
> category of checks (e.g.: a guideline) is not applicable to the code. In this 
> case aliases should not be disabled. So I think a global option would not 
> solve this issue. A better solution might be a per glob notation. So `-` 
> could mean do not disable aliases and `--` could mean disable aliases as well 
> but that is just an example.
>
> All in all, I think this is not related strictly to this issue and I think we 
> should discuss this separately on the mailing list.
>
> In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote:
>
> > (Also, if the check has parameters, and one specifies different parameters 
> > for the base check, and the aliases, what happens?)
>
>
> I think this is a very good point, this needs to be documented somewhere.


A mail [0] posted by @JonasToth is about the similar problem, i think we can 
continue there.

I *think* he has a point, a some better way to setup aliases, without actually
creating aliased checks is the solution IMO. This way, there won't be any
need to disable all the aliases when completely disabling the checks, and
there won't be multiple config options for the each alias. The legacy support
is the question though.

[0] (https://lists.llvm.org/pipermail/cfe-dev/2017-September/055589.html)


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] D38171: Implement clang-tidy check aliases.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D38171#880025, @xazax.hun wrote:

> In https://reviews.llvm.org/D38171#880022, @lebedev.ri wrote:
>
> > A mail [0] posted by @JonasToth is about the similar problem, i think we 
> > can continue there.
>
>
> Great! Could you summarize your points there as well? Thanks in advance.


Done. 

In https://reviews.llvm.org/D38171#880024, @alexfh wrote:

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


It's much like clang's `-Weverything`, which is rather convenient, and is
inconveniently 'missing 
' in gcc, because the 
devs believe that there would
be complains that it results in new warnings each release, 
which is kinda the point of that flag...

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

Except the disagreeing checks, same could be said about `-Weverything`, 
clang-analyzer,
etc :)   Yes, of course it does not just work, and of course needs some further 
tuning,
and of course the codebase that is being checked needs to be fixed not to warn.

I.e. the use-case is to always have all the warnings enabled, and each time a 
new check
is triggered, analyze whether it should be kept enabled, or disable it.


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] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 116857.
lebedev.ri added a comment.

Rebased, ping.

Vanilla stage-2 build is now warning-clean, the only previous warning was fixed.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c

Index: test/Sema/outof-range-constant-compare.c
===
--- test/Sema/outof-range-constant-compare.c
+++ test/Sema/outof-range-constant-compare.c
@@ -102,6 +102,7 @@
   return 1;
 
 short s = value();
+
 if (s == 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always false}}
 return 0;
 if (s != 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always true}}
@@ -128,6 +129,112 @@
 if (0x1234567812345678L >= s) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always true}}
 return 0;
 
+if (s == 32767)
+return 0;
+if (s != 32767)
+return 0;
+if (s < 32767)
+return 0;
+if (s <= 32767) // expected-warning {{comparison 'short' <= 32767 is always true}}
+return 0;
+if (s > 32767) // expected-warning {{comparison 'short' > 32767 is always false}}
+return 0;
+if (s >= 32767)
+return 0;
+
+if (32767 == s)
+return 0;
+if (32767 != s)
+return 0;
+if (32767 < s) // expected-warning {{comparison 32767 < 'short' is always false}}
+return 0;
+if (32767 <= s)
+return 0;
+if (32767 > s)
+return 0;
+if (32767 >= s) // expected-warning {{comparison 32767 >= 'short' is always true}}
+return 0;
+
+// FIXME: assumes two's complement
+if (s == -32768)
+return 0;
+if (s != -32768)
+return 0;
+if (s < -32768) // expected-warning {{comparison 'short' < -32768 is always false}}
+return 0;
+if (s <= -32768)
+return 0;
+if (s > -32768)
+return 0;
+if (s >= -32768) // expected-warning {{comparison 'short' >= -32768 is always true}}
+return 0;
+
+if (-32768 == s)
+return 0;
+if (-32768 != s)
+return 0;
+if (-32768 < s)
+return 0;
+if (-32768 <= s) // expected-warning {{comparison -32768 <= 'short' is always true}}
+return 0;
+if (-32768 > s) // expected-warning {{comparison -32768 > 'short' is always false}}
+return 0;
+if (-32768 >= s)
+return 0;
+
+if (s == 32767UL)
+return 0;
+if (s != 32767UL)
+return 0;
+if (s < 32767UL)
+return 0;
+if (s <= 32767UL) // expected-warning {{comparison 'short' <= 32767 is always true}}
+return 0;
+if (s > 32767UL) // expected-warning {{comparison 'short' > 32767 is always false}}
+return 0;
+if (s >= 32767UL)
+return 0;
+
+if (32767UL == s)
+return 0;
+if (32767UL != s)
+return 0;
+if (32767UL < s) // expected-warning {{comparison 32767 < 'short' is always false}}
+return 0;
+if (32767UL <= s)
+return 0;
+if (32767UL > s)
+return 0;
+if (32767UL >= s) // expected-warning {{comparison 32767 >= 'short' is always true}}
+return 0;
+
+// FIXME: assumes two's complement
+if (s == -32768L)
+return 0;
+if (s != -32768L)
+return 0;
+if (s < -32768L) // expected-warning {{comparison 'short' < -32768 is always false}}
+return 0;
+if (s <= -32768L)
+return 0;
+if (s > -32768L)
+return 0;
+if (s >= -32768L) // expected-warning {{comparison 'short' >= -32768 is always true}}
+return 0;
+
+if (-32768L == s)
+return 0;
+if (-32768L != s)
+return 0;
+if (-32768L < s)
+return 0;
+if (-32768L <= s) // expected-warning {{comparison -32768 <= 'short' is always true}}
+return 0;
+if (-32768L > s) // expected-warning {{comparison -32768 > 'short' is always false}}
+return 0;
+if (-32768L >= s)
+return 0;
+
 long l = value();
 if (l == 0x1234567812345678L)
 return 0;
@@ -208,6 +315,66 @@
 if (0xUL >= un)
 return 0;
 
+unsigned short us = value();
+
+if (us == 65535)
+return 0;
+if (us != 65535)
+return 0;
+if (us < 65535)
+return 0;
+if (us <= 65535) // expected-warning {{comparison 'unsigned short' <= 65535 is always true}}
+return 0;
+if (us > 65535) // expected-warning {{comparison 'unsigned short' > 65535 is always false}}
+return 0;
+if (us >= 65535)
+return 0;
+
+if (65535 == us)
+return 0;
+if (65535 != us)
+ 

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ping.

In https://reviews.llvm.org/D36836#877642, @lebedev.ri wrote:

> In https://reviews.llvm.org/D36836#877636, @alexfh wrote:
>
> > > The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource 
> > > specification version 1.2 (19 April 2017),
> >
> > Err, "I did ask them, and received an answer that it is it can be 
> > implemented in clang-tidy" might not be enough.
>
>
> F5372479: original_msg.txt 
>
>Hello Roman,
>   
>It would not, and I would be happy to be notified once done.
>   
>Thanks
>   
>Olivier
>   
>   > On Fri, Jul 28, 2017 at 6:17 PM Roman Lebedev  
> wrote:
>   > Hi.
>   > 
>   > I would like to know, what is the legal status of
>   > https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
>   > 
>   > If i (or someone else) is to implement the Specification described in
>   > that paper,
>   > which will produce the same results as the Cognitive Complexity in 
> SonarSource
>   > as part of some other static analyzer, for example as a LLVM's clang-tidy 
> check,
>   > would that be illegal thing to do?
>   > 
>   > Roman.
>-- 
>Olivier Gaudin | SonarSource
>CEO & Co-Founder
>+41 (0)22 510 2424
>http://sonarsource.com
>   
>
> Might not be enough?


@alexfh please specify, is that enough or not?


Repository:
  rL LLVM

https://reviews.llvm.org/D36836



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ping :)
Does this need more, different tests?
Should i rewrite that lengthy if-else-if chain somehow differently?


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 117710.
lebedev.ri marked 6 inline comments as done.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.

@rsmith thank you for the review!

Address review notes:

1. Merge `CheckTautologicalComparisonWithZero()` and 
`CheckTautologicalComparisonWithMinMax()`
2. Significantly deduplicate the code, hopefully not at the cost of readability.
3. Slightly cleanup/move around/update relevant tests for these three types of 
tautological comparisons.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c
  test/Sema/tautological-constant-compare.c
  test/Sema/tautological-unsigned-zero-compare.c

Index: test/Sema/tautological-unsigned-zero-compare.c
===
--- test/Sema/tautological-unsigned-zero-compare.c
+++ test/Sema/tautological-unsigned-zero-compare.c
@@ -1,47 +1,340 @@
 // RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify -x c++ %s
 
-unsigned value(void);
+unsigned uvalue(void);
+signed int svalue(void);
 
 int main() {
-  unsigned un = value();
+  unsigned un = uvalue();
 
 #ifdef TEST
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
+  return 0;
   if (0 > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
-  if (un < 0U) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
-  if (un >= 0U) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
-  if (0U <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
-  if (0U > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+  return 0;
+  if (0UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+  return 0;
+  if (0UL >= un)
+  return 0;
 #else
 // expected-no-diagnostics
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0)
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0)
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un)
-return 0;
+  return 0;
   if (0 > un)
-return 0;
-  if (un < 0U)
-return 0;
-  if (un >= 0U)
-return 0;
-  if (0U <= un)
-return 0;
-  if (0U > un)
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL)
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL)
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un)
+  return 0;
+  if (0UL > un)
+  return 0;
+  if (0UL >= un)
+  return 0;
 #endif
 
+
+  signed int a = svalue();
+
+#ifdef TEST
+  if (a == 0)
+  return 0;
+  if (a != 0)
+  return 0;
+  if (a < 0)
+  return 0;
+  if (a <= 0)
+  return 0;
+  if (a > 0)
+  return 0;
+  if (a >= 0)
+  return 0;
+
+  if (0 == a)
+  return 0;
+  if (0 != a)
+  return 0;
+  if (0 < a)
+  return 0;
+  if (0 <= a)
+  return 0;
+  if (0 > a)
+  return 0;
+  if (0 >= a)
+  return 0;
+
+  if (a == 0UL)
+  return 0;
+  if (a != 

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:8697
+
+  return true;
 }

If the diagnostic we are about to output is disabled, should we still `return 
true;` and suppress potential `-Wsign-compare` warning?


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 117802.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a subscriber: rtrieu.
lebedev.ri added a comment.

- Address review notes
- ~~Avoid re-evaluating ICE when called from `DiagnoseOutOfRangeComparison()` 
which already knows the evaluated value~~

  Don't even check if this is a Type Limit in `DiagnoseOutOfRangeComparison()`. 
Perhaps @rtrieu can shed some light on this.
- Enhance test coverage a little (add a test with macro, with template)


Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c
  test/Sema/tautological-constant-compare.c
  test/Sema/tautological-unsigned-zero-compare.c

Index: test/Sema/tautological-unsigned-zero-compare.c
===
--- test/Sema/tautological-unsigned-zero-compare.c
+++ test/Sema/tautological-unsigned-zero-compare.c
@@ -1,47 +1,370 @@
 // RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify -x c++ %s
 
-unsigned value(void);
+unsigned uvalue(void);
+signed int svalue(void);
 
-int main() {
-  unsigned un = value();
+#define macro(val) val
+
+#ifdef __cplusplus
+template
+void TFunc() {
+  // Make sure that we do warn for normal variables in template functions !
+  unsigned char c = svalue();
+#ifdef TEST
+  if (c < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return;
+#else
+  if (c < 0)
+  return;
+#endif
+
+  if (c < macro(0))
+  return;
+
+  T v = svalue();
+  if (v < 0)
+  return;
+}
+#endif
+
+int main()
+{
+#ifdef __cplusplus
+  TFunc();
+  TFunc();
+#endif
+
+  unsigned un = uvalue();
 
 #ifdef TEST
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
+  return 0;
   if (0 > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
-  if (un < 0U) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
-  if (un >= 0U) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
-  if (0U <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
-  if (0U > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+  return 0;
+  if (0UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+  return 0;
+  if (0UL >= un)
+  return 0;
 #else
 // expected-no-diagnostics
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0)
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0)
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un)
-return 0;
+  return 0;
   if (0 > un)
-return 0;
-  if (un < 0U)
-return 0;
-  if (un >= 0U)
-return 0;
-  if (0U <= un)
-return 0;
-  if (0U > un)
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL)
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL)
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un)
+  return 0;
+  if (0UL > un)
+  return 0;
+  if (0UL >=

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:8719
+  // Type limit values are handled later by CheckTautologicalComparison().
+  if (IsTypeLimit(S, Other, Constant, ConstValue) && (!OtherIsBooleanType))
 return;

rsmith wrote:
> Is this necessary? (Aren't the type limit values always within the range of 
> the type in question?)
> 
> Can we avoid evaluating `Constant` a extra time here? (We already have its 
> value in `Value`.)
Uhm, good question :)
If i remove this, `check-clang-sema` and `check-clang-semacxx` still pass.
I agree that it does not make much sense. Initially it was only checking for 
`Value == 0`.
Git suggests that initially this branch was added by @rtrieu, maybe can help.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 117824.
lebedev.ri added a comment.

Ping.

- Rebased
- Added the legal status info into `LICENSE.txt`, referencing 
`clang-tidy/readability/FunctionCognitiveComplexityCheck.LICENSE.txt`, as 
suggested by @aaron.ballman

Question:
Is it expected that clang-tidy somehow parses the `DiagnosticIDs::Note`'s as 
`FixIt`'s, and thus fails with the following assert:

  Running ['clang-tidy', 
'/build/llvm-build-Clang-release/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp.cpp',
 '-fix', '--checks=-*,readability-function-cognitive-complexity', 
'-config={CheckOptions: [{key: 
readability-function-cognitive-complexity.Threshold, value: 0}]}', '--', 
'-std=c++11', '-nostdinc++']...
  clang-tidy failed:
  Fix conflicts with existing fix! The new replacement overlaps with an 
existing replacement.
  New replacement: 
/build/llvm-build-Clang-release/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp.cpp:
 3484:+5:""
  Existing replacement: 
/build/llvm-build-Clang-release/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp.cpp:
 3485:+2:"&"
  clang-tidy: 
/build/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:86: virtual 
void (anonymous 
namespace)::ClangTidyDiagnosticRenderer::emitCodeContext(clang::FullSourceLoc, 
DiagnosticsEngine::Level, SmallVectorImpl &, 
ArrayRef): Assertion `false && "Fix conflicts with existing 
fix!"' failed.


Repository:
  rL LLVM

https://reviews.llvm.org/D36836

Files:
  LICENSE.TXT
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/FunctionCognitiveComplexityCheck.LICENSE.txt
  clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
  clang-tidy/readability/FunctionCognitiveComplexityCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-function-cognitive-complexity.rst
  test/clang-tidy/readability-function-cognitive-complexity.cpp

Index: test/clang-tidy/readability-function-cognitive-complexity.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-function-cognitive-complexity.cpp
@@ -0,0 +1,952 @@
+// RUN: %check_clang_tidy %s readability-function-cognitive-complexity %t -- -config='{CheckOptions: [{key: readability-function-cognitive-complexity.Threshold, value: 0}]}' -- -std=c++11
+
+// any function should be checked.
+
+extern int ext_func(int x = 0);
+
+int some_func(int x = 0);
+
+static int some_other_func(int x = 0) {}
+
+template void some_templ_func(T x = 0) {}
+
+class SomeClass {
+public:
+  int *begin(int x = 0);
+  int *end(int x = 0);
+  static int func(int x = 0);
+  template void some_templ_func(T x = 0) {}
+};
+
+// nothing ever decreases cognitive complexity, so we can check all the things
+// in one go. none of the following should increase cognitive complexity:
+void unittest_false() {
+  {};
+  ext_func();
+  some_func();
+  some_other_func();
+  some_templ_func();
+  some_templ_func();
+  SomeClass::func();
+  SomeClass C;
+  C.some_templ_func();
+  C.some_templ_func();
+  C.func();
+  C.end();
+  int i = some_func();
+  i = i;
+  i++;
+  --i;
+  i < 0;
+  int j = 0 ?: 1;
+  auto k = new int;
+  delete k;
+  throw i;
+  {
+throw i;
+  }
+end:
+  return;
+}
+
+#if 1
+#define CC100
+#else
+// this macro has cognitive complexity of 100.
+// it is needed to be able to compare the testcases with the
+// reference Sonar implementation. please place it right after the first
+// CHECK-NOTES in each function
+#define CC100 if(1){if(1){if(1){if(1){if(1){if(1){if(1){if(1){if(1){if(1){if(1){if(1){if(1){}if(1){}
+#endif
+
+////
+//-- B1. Increments --//
+////
+// Check that every thing listed in B1 of the specification does indeed   //
+// recieve the base increment, and that not-body does not increase nesting//
+////
+
+// break does not increase cognitive complexity.
+// only  break LABEL  does, but it is unavaliable in C or C++
+
+// continue does not increase cognitive complexity.
+// only  continue LABEL  does, but it is unavaliable in C or C++
+
+void unittest_b1_00() {
+// CHECK-NOTES: :[[@LINE-1]]:6: warning: function 'unittest_b1_00' has cognitive complexity of 33 (threshold 0) [readability-function-cognitive-complexity]
+  CC100;
+
+  if (1 ? 1 : 0) {
+// CHECK-NOTES: :[[@LINE-1]]:3: note: +1, including nesting penalty of 0, nesting level increased to 1{{$}}
+// CHECK-NOTES: :[[@LINE-2]]:9: note: +1, including nesting penalty of 0, nesting level increased to 1{{$}}
+
+if (1 ? 1 : 0) {
+// 

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:8719
+  // Type limit values are handled later by CheckTautologicalComparison().
+  if (IsTypeLimit(S, Other, Constant, ConstValue) && (!OtherIsBooleanType))
 return;

lebedev.ri wrote:
> rsmith wrote:
> > Is this necessary? (Aren't the type limit values always within the range of 
> > the type in question?)
> > 
> > Can we avoid evaluating `Constant` a extra time here? (We already have its 
> > value in `Value`.)
> Uhm, good question :)
> If i remove this, `check-clang-sema` and `check-clang-semacxx` still pass.
> I agree that it does not make much sense. Initially it was only checking for 
> `Value == 0`.
> Git suggests that initially this branch was added by @rtrieu, maybe can help.
[[ 
https://github.com/llvm-mirror/clang/commit/526e627d2bd7e8cbf630526d315c90864898d9ff#diff-93faf32157a807b1b7953f3747db08b6R4332
 | The most original version of this code ]]
After some though i think the initial check `Value == 0` was simply to quickly 
bail out
out of `DiagnoseOutOfRangeComparison()`, and not waste any time for the obvious 
case
(`0`), which can't be out-of-range, ever. So i think the right behaviour could 
be:
1. Either simply restore the original check:
```
// 0 values are handled later by CheckTautologicalComparison().
if ((Value == 0) && (!OtherIsBooleanType))
  return;
```
And add a comment there about it
2. Or, drop it completely
3. Or, perhaps refactor `CheckTautologicalComparison()`, and more or less call 
it from
 function `AnalyzeComparison()`, before calling 
`DiagnoseOutOfRangeComparison()`,
 thus completely avoiding the need to re-evaluate `Constant` there later on,
 and simplify the logic in the process.

I personally think the `3.` *might* be best.
WDYT?


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:8719
+  // Type limit values are handled later by CheckTautologicalComparison().
+  if (IsTypeLimit(S, Other, Constant, ConstValue) && (!OtherIsBooleanType))
 return;

lebedev.ri wrote:
> lebedev.ri wrote:
> > rsmith wrote:
> > > Is this necessary? (Aren't the type limit values always within the range 
> > > of the type in question?)
> > > 
> > > Can we avoid evaluating `Constant` a extra time here? (We already have 
> > > its value in `Value`.)
> > Uhm, good question :)
> > If i remove this, `check-clang-sema` and `check-clang-semacxx` still pass.
> > I agree that it does not make much sense. Initially it was only checking 
> > for `Value == 0`.
> > Git suggests that initially this branch was added by @rtrieu, maybe can 
> > help.
> [[ 
> https://github.com/llvm-mirror/clang/commit/526e627d2bd7e8cbf630526d315c90864898d9ff#diff-93faf32157a807b1b7953f3747db08b6R4332
>  | The most original version of this code ]]
> After some though i think the initial check `Value == 0` was simply to 
> quickly bail out
> out of `DiagnoseOutOfRangeComparison()`, and not waste any time for the 
> obvious case
> (`0`), which can't be out-of-range, ever. So i think the right behaviour 
> could be:
> 1. Either simply restore the original check:
> ```
> // 0 values are handled later by CheckTautologicalComparison().
> if ((Value == 0) && (!OtherIsBooleanType))
>   return;
> ```
> And add a comment there about it
> 2. Or, drop it completely
> 3. Or, perhaps refactor `CheckTautologicalComparison()`, and more or less 
> call it from
>  function `AnalyzeComparison()`, before calling 
> `DiagnoseOutOfRangeComparison()`,
>  thus completely avoiding the need to re-evaluate `Constant` there later 
> on,
>  and simplify the logic in the process.
> 
> I personally think the `3.` *might* be best.
> WDYT?
Tried implementing `3.`.
It won't work, because `DiagnoseOutOfRangeComparison()` needs the `{L,R}HS`
after `IgnoreParenImpCasts()`, and `CheckTautologicalComparison()` is not ok 
with that.
It seems that at most, i could re-use the detection of `RhsConstant`.

So, new options:
1. Either simply restore the original check, and add a comment there about the 
logic behind it
2. Or, drop the check completely
3. Or, move the  `CheckTautologicalComparison()` call before 
`DiagnoseOutOfRangeComparison()`
 And if `DiagnoseOutOfRangeComparison()` has already emitted diagnostic, 
return.
 Much like what `CheckTautologicalComparison()` already does.
 
So i think `3.` is still the best option :)
(tried implementing it, appears to work)


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 118143.
lebedev.ri added a comment.

Rework `AnalyzeComparison()` and `DiagnoseOutOfRangeComparison()` - avoid that 
fast-return path in `DiagnoseOutOfRangeComparison()`
`AnalyzeComparison()` is now a bit smarter, and it

1. calls `CheckTautologicalComparison()`
2. returns if it diagnosed anything
3. else, calls `DiagnoseOutOfRangeComparison()`
4. returns if it diagnosed anything.

`check-clang-sema` and `check-clang-semacxx` are good.
Stage-2 build still good.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c
  test/Sema/tautological-constant-compare.c
  test/Sema/tautological-unsigned-zero-compare.c

Index: test/Sema/tautological-unsigned-zero-compare.c
===
--- test/Sema/tautological-unsigned-zero-compare.c
+++ test/Sema/tautological-unsigned-zero-compare.c
@@ -1,47 +1,370 @@
 // RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify -x c++ %s
 
-unsigned value(void);
+unsigned uvalue(void);
+signed int svalue(void);
 
-int main() {
-  unsigned un = value();
+#define macro(val) val
+
+#ifdef __cplusplus
+template
+void TFunc() {
+  // Make sure that we do warn for normal variables in template functions !
+  unsigned char c = svalue();
+#ifdef TEST
+  if (c < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return;
+#else
+  if (c < 0)
+  return;
+#endif
+
+  if (c < macro(0))
+  return;
+
+  T v = svalue();
+  if (v < 0)
+  return;
+}
+#endif
+
+int main()
+{
+#ifdef __cplusplus
+  TFunc();
+  TFunc();
+#endif
+
+  unsigned un = uvalue();
 
 #ifdef TEST
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
+  return 0;
   if (0 > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
-  if (un < 0U) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
-  if (un >= 0U) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
-  if (0U <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
-  if (0U > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+  return 0;
+  if (0UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+  return 0;
+  if (0UL >= un)
+  return 0;
 #else
 // expected-no-diagnostics
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0)
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0)
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un)
-return 0;
+  return 0;
   if (0 > un)
-return 0;
-  if (un < 0U)
-return 0;
-  if (un >= 0U)
-return 0;
-  if (0U <= un)
-return 0;
-  if (0U > un)
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL)
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL)
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un)
+  return 0;
+  if (0UL > un)
+  return 0;
+  if (0UL 

[PATCH] D38718: Patch to Bugzilla 20951

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: test/Sema/conditional-expr.c:84
+  //char x;
+  return (((x != ((void *) 0)) ? (*x = ((char) 1)) : (void) ((void *) 0)), 
(unsigned long) ((void *) 0)); // expected-warning {{C99 forbids conditional 
expressions with only one void side}}
 }

Please don't just remove previous tests.
E.g. does the old test no longer warns?



https://reviews.llvm.org/D38718



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 118793.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.

@rsmith, thank you for the review!

Rebased, address review notes, simplified all the things even further.

Testing:

- `$ ninja check-clang-sema check-clang-semacxx`
- Stage 2 build (still good, no issues/warnings)


Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c
  test/Sema/tautological-constant-compare.c
  test/Sema/tautological-unsigned-zero-compare.c

Index: test/Sema/tautological-unsigned-zero-compare.c
===
--- test/Sema/tautological-unsigned-zero-compare.c
+++ test/Sema/tautological-unsigned-zero-compare.c
@@ -1,47 +1,370 @@
 // RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify -x c++ %s
 
-unsigned value(void);
+unsigned uvalue(void);
+signed int svalue(void);
 
-int main() {
-  unsigned un = value();
+#define macro(val) val
+
+#ifdef __cplusplus
+template
+void TFunc() {
+  // Make sure that we do warn for normal variables in template functions !
+  unsigned char c = svalue();
+#ifdef TEST
+  if (c < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return;
+#else
+  if (c < 0)
+  return;
+#endif
+
+  if (c < macro(0))
+  return;
+
+  T v = svalue();
+  if (v < 0)
+  return;
+}
+#endif
+
+int main()
+{
+#ifdef __cplusplus
+  TFunc();
+  TFunc();
+#endif
+
+  unsigned un = uvalue();
 
 #ifdef TEST
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
+  return 0;
   if (0 > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
-  if (un < 0U) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
-  if (un >= 0U) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
-  if (0U <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
-  if (0U > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+  return 0;
+  if (0UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+  return 0;
+  if (0UL >= un)
+  return 0;
 #else
 // expected-no-diagnostics
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0)
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0)
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un)
-return 0;
+  return 0;
   if (0 > un)
-return 0;
-  if (un < 0U)
-return 0;
-  if (un >= 0U)
-return 0;
-  if (0U <= un)
-return 0;
-  if (0U > un)
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL)
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL)
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un)
+  return 0;
+  if (0UL > un)
+  return 0;
+  if (0UL >= un)
+  return 0;
 #endif
 
+
+  signed int a = svalue();
+
+#ifdef TEST
+  if (a == 0)
+  return 0;
+  if (a != 0)
+  return 0;
+  if (a < 0)
+  retu

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:8667-8679
+  bool Result; // The result of the comparison
+  if ((Op == BO_GT && ValueType == LimitType::Max && ConstOnRight) ||
+  (Op == BO_GT && ValueType == LimitType::Min && !ConstOnRight) ||
+  (Op == BO_LT && ValueType == LimitType::Max && !ConstOnRight) ||
+  (Op == BO_LT && ValueType == LimitType::Min && ConstOnRight)) {
+Result = false;
+  } else if ((Op == BO_GE && ValueType == LimitType::Max && !ConstOnRight) ||

rsmith wrote:
> The exhaustive case analysis is a little hard to verify. Perhaps something 
> like this would be clearer:
> 
> ```
> bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ ConstOnRight;
> bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
> bool ResultWhenConstNeOther = ConstIsLowerBound ^ ValueType == LimitType::Max;
> if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
>   return false;
> ```
Oh, that looks better indeed :)
I did consider doing something like that, but my variant ended up looking worse 
than even the current `if()`'s



Comment at: lib/Sema/SemaChecking.cpp:8940
+  } else if (!T->hasUnsignedIntegerRepresentation())
+IsComparisonConstant = E->isIntegerConstantExpr(S.Context);
+

rsmith wrote:
> It seems suboptimal to evaluate both sides of the comparison and then 
> evaluate the entire comparison again in this case. Presumably the point is to 
> catch comparisons whose operands are not of integral type (eg, floating 
> point), but we could get that benefit and also catch more integral cases by 
> switching from `isIntegerConstantExpr` to the more appropriate 
> `EvaluateAsRValue`.
> 
> I'm fine with a cleanup to avoid the repeated evaluation here being deferred 
> to a later patch.
If we look at this code closely, if `hasUnsignedIntegerRepresentation() == 
false`, we always do `return AnalyzeImpConvsInComparison(S, E);`, regardless of 
`E->isIntegerConstantExpr(S.Context)`.
So if i move more stuff into `if (T->isIntegralType(S.Context))`, then ` 
E->isIntegerConstantExpr(S.Context);` is simply gone.
At least the current tests say so.



Comment at: lib/Sema/SemaChecking.cpp:8949
+  if (T->isIntegralType(S.Context)) {
+if (CheckTautologicalComparison(S, E))
+  return AnalyzeImpConvsInComparison(S, E);

rsmith wrote:
> Pass `LHSValue` and `RHSValue` in here rather than recomputing them.
We can even go one step further, and pass the `bool IsConstLiteralOnRHS`


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D36836#889375, @aaron.ballman wrote:

> Adding @dberlin for licensing discussion questions.


Ping.

Yes, i agree that what i have added is not a directory, and not a proper 
license.
That is more of a template to hopefully stat moving things in the right 
direction.

To clarify, my problem with this Differential is that i'we yet to see any 
opinion on
the proposed check from the people who will actually have the final say whether
it will be accepted or not. It will be *really* sad if we go through all this 
trouble 
with the legal matters, and then it will be NACK'ed...


Repository:
  rL LLVM

https://reviews.llvm.org/D36836



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 118829.
lebedev.ri marked an inline comment as done.

Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c
  test/Sema/tautological-constant-compare.c
  test/Sema/tautological-unsigned-zero-compare.c

Index: test/Sema/tautological-unsigned-zero-compare.c
===
--- test/Sema/tautological-unsigned-zero-compare.c
+++ test/Sema/tautological-unsigned-zero-compare.c
@@ -1,47 +1,370 @@
 // RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify -x c++ %s
 
-unsigned value(void);
+unsigned uvalue(void);
+signed int svalue(void);
 
-int main() {
-  unsigned un = value();
+#define macro(val) val
+
+#ifdef __cplusplus
+template
+void TFunc() {
+  // Make sure that we do warn for normal variables in template functions !
+  unsigned char c = svalue();
+#ifdef TEST
+  if (c < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return;
+#else
+  if (c < 0)
+  return;
+#endif
+
+  if (c < macro(0))
+  return;
+
+  T v = svalue();
+  if (v < 0)
+  return;
+}
+#endif
+
+int main()
+{
+#ifdef __cplusplus
+  TFunc();
+  TFunc();
+#endif
+
+  unsigned un = uvalue();
 
 #ifdef TEST
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
+  return 0;
   if (0 > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
-  if (un < 0U) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
-  if (un >= 0U) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
-  if (0U <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
-  if (0U > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+  return 0;
+  if (0UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+  return 0;
+  if (0UL >= un)
+  return 0;
 #else
 // expected-no-diagnostics
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0)
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0)
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un)
-return 0;
+  return 0;
   if (0 > un)
-return 0;
-  if (un < 0U)
-return 0;
-  if (un >= 0U)
-return 0;
-  if (0U <= un)
-return 0;
-  if (0U > un)
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL)
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL)
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un)
+  return 0;
+  if (0UL > un)
+  return 0;
+  if (0UL >= un)
+  return 0;
 #endif
 
+
+  signed int a = svalue();
+
+#ifdef TEST
+  if (a == 0)
+  return 0;
+  if (a != 0)
+  return 0;
+  if (a < 0)
+  return 0;
+  if (a <= 0)
+  return 0;
+  if (a > 0)
+  return 0;
+  if (a >= 0)
+  return 0;
+
+  if (0 == a)
+  return 0;
+  if (0 != a)
+  return 0;
+  if (0 < a)
+  return 0;
+  if (0 <= a)
+  return 0;
+  if (0 > a)
+ 

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:8930
+// We only care about expressions where just one side is literal
+if (IsRHSIntegralLiteral ^ IsLHSIntegralLiteral) {
+  // Is the constant on the RHS or LHS?

rsmith wrote:
> It would probably be more obvious to use `||` here, since you already 
> returned in the case where both sides are constant.
And what if neither of them is constant?
I *think* we could reach this point in that case.
At least it is not immediately obvious to me why it can't happen.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 118833.
lebedev.ri added a comment.

Getting really weird problems when trying to use arc patch, maybe re-updating 
the differential helps


Repository:
  rL LLVM

https://reviews.llvm.org/D38101

Files:
  docs/ReleaseNotes.rst
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/outof-range-constant-compare.c
  test/Sema/tautological-constant-compare.c
  test/Sema/tautological-unsigned-zero-compare.c

Index: test/Sema/tautological-unsigned-zero-compare.c
===
--- test/Sema/tautological-unsigned-zero-compare.c
+++ test/Sema/tautological-unsigned-zero-compare.c
@@ -1,47 +1,370 @@
 // RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify -x c++ %s
 
-unsigned value(void);
+unsigned uvalue(void);
+signed int svalue(void);
 
-int main() {
-  unsigned un = value();
+#define macro(val) val
+
+#ifdef __cplusplus
+template
+void TFunc() {
+  // Make sure that we do warn for normal variables in template functions !
+  unsigned char c = svalue();
+#ifdef TEST
+  if (c < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return;
+#else
+  if (c < 0)
+  return;
+#endif
+
+  if (c < macro(0))
+  return;
+
+  T v = svalue();
+  if (v < 0)
+  return;
+}
+#endif
+
+int main()
+{
+#ifdef __cplusplus
+  TFunc();
+  TFunc();
+#endif
+
+  unsigned un = uvalue();
 
 #ifdef TEST
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
+  return 0;
   if (0 > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
-  if (un < 0U) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-return 0;
-  if (un >= 0U) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-return 0;
-  if (0U <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-return 0;
-  if (0U > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+  return 0;
+  if (0UL > un) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+  return 0;
+  if (0UL >= un)
+  return 0;
 #else
 // expected-no-diagnostics
+  if (un == 0)
+  return 0;
+  if (un != 0)
+  return 0;
   if (un < 0)
-return 0;
+  return 0;
+  if (un <= 0)
+  return 0;
+  if (un > 0)
+  return 0;
   if (un >= 0)
-return 0;
+  return 0;
+
+  if (0 == un)
+  return 0;
+  if (0 != un)
+  return 0;
+  if (0 < un)
+  return 0;
   if (0 <= un)
-return 0;
+  return 0;
   if (0 > un)
-return 0;
-  if (un < 0U)
-return 0;
-  if (un >= 0U)
-return 0;
-  if (0U <= un)
-return 0;
-  if (0U > un)
-return 0;
+  return 0;
+  if (0 >= un)
+  return 0;
+
+  if (un == 0UL)
+  return 0;
+  if (un != 0UL)
+  return 0;
+  if (un < 0UL)
+  return 0;
+  if (un <= 0UL)
+  return 0;
+  if (un > 0UL)
+  return 0;
+  if (un >= 0UL)
+  return 0;
+
+  if (0UL == un)
+  return 0;
+  if (0UL != un)
+  return 0;
+  if (0UL < un)
+  return 0;
+  if (0UL <= un)
+  return 0;
+  if (0UL > un)
+  return 0;
+  if (0UL >= un)
+  return 0;
 #endif
 
+
+  signed int a = svalue();
+
+#ifdef TEST
+  if (a == 0)
+  return 0;
+  if (a != 0)
+  return 0;
+  if (a < 0)
+  return 0;
+  if (a <= 0)
+  return 0;
+  if (a > 0)
+  return 0;
+  if (a >= 0)
+  return 0;
+
+  if (0 == a)
+  return 0;
+  if (0 != a)
+  re

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri reopened this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Reverted due to http://bb9.pgr.jp/#/builders/20/builds/59 that i don't 
currently know how to deal with.
It is really sad that i failed to encounter it during testing.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: cfe/trunk/test/Sema/tautological-constant-compare.c:23
+
+  if (c > macro(255))
+  return;

I'm having second thoughts about macro handling.
Right now we completely ignore the comparisons when the constant is anyhow 
involved with macros.
I.e.
```
unsigned char c;
if (c > 255) // expected-warning {{comparison 'unsigned char' > 255 is always 
false}}
  return;  // would be diagnosed correctly
// but
assert (c <= 255); // would be completely ignored.
```
Perhaps we should be a bit more strict, and should not bailout in such cases?

But i *guess* this case we still should ignore
```
#define macro(val) (val > 255)
if (macro(c))
  return;
```

(Even though gcc warns in the last variant too)


Repository:
  rL LLVM

https://reviews.llvm.org/D38101



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