[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked an inline comment as done.
JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:147
+  // Example: `int i = 10`, `int i` (will be used if program is correct)
+  const auto LocalValDecl = varDecl(unless(anyOf(
+  isLocal(), hasInitializer(anything()), unless(ConstType),

aaron.ballman wrote:
> JonasToth wrote:
> > @aaron.ballman The change was not valid for some reason. I leave it like it 
> > is if thats ok with you.
> That's really odd. I am fine leaving it as-is for this patch, but it 
> would be good to understand why that code fails as it seems like a reasonable 
> exposition.
Added a TODO. But maybe i did the transformation incorrectly too. Not all 
conditions are negative. Maybe all the negative ones can be inverted by 
demorgan. That should be correct.

I will check this out later.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked an inline comment as done.
JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:183
+  // TODO Implement automatic code transformation to add the 'const'.
+  diag(Variable->getLocStart(), "variable %0 of type %1 can be declared const")
+  << Variable << Variable->getType();

aaron.ballman wrote:
> Still missing the single quotes around `const` in the diagnostic.
Ups. The comment has them :D


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 159390.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.

- address review issues, todos/fixmes and diag nit


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,557 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared const
+np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = &np_local0;
+  int *const p1_np_local0 = &np_local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = &np_local1;
+  int *const p1_np_local1 = &np_local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(&np_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = &np_local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared const
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int &r0_np_local0 = np_local0;
+  int &r1_np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return &np_local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3:

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D45444#1189262, @aaron.ballman wrote:

> However, I'm wondering how this should integrate with other const-correctness 
> efforts like `readability-non-const-parameter`?


I think this check/functionality will kinda replace the 
`readability-non-const-parameter` check. The readability check does not a full 
const-analysis too and i think only works on pointers or sth like this.
Maybe the check name will still exist, but use the `ExprMutAnalyzer` or it will 
become an alias to this with special configuration.
I would like to add support for marking methods `const` plus the ability for 
code transformation. Currently looking into `clang-refactor` framework to 
implement general refactoring primitives necessary for that.
In general its probably better to have one check, that handles all `const` 
issues.

> Also, I'm wondering how this check performs over a large code base like LLVM 
> -- how chatty are the diagnostics, and how bad is the false positive rate 
> (roughly)?

I will prepare a report for this tomorrow. Currently the LLVM builds take very 
long on my laptop :(


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I would like to have this patch in, because i currently have a 1MB output file, 
and the missing synchronization really destroys cleaning with grep and sed :(




Comment at: clang-tidy/tool/run-clang-tidy.py:167
+output, err = proc.communicate()
+if proc.returncode:
   failed_files.append(name)

Please do the comparison `!= 0` to be more clear about the expected success 
return value.



Comment at: clang-tidy/tool/run-clang-tidy.py:171
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if len(err):
+sys.stderr.write(err + '\n')

Please add a comparison you are looking for (`> 0` i guess)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I forgot: Did you measure how much of a time penalty this brings? Just out of 
curiosity, correct script is more important then fast script :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Lets say running it over whole LLVM for one check? I will apply this path 
locally, because i have to analyze llvm right now anyway.

For me this looks good, but @alexfh or @aaron.ballman should accept it.


https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I do run it over llvm/lib, there is no visible effect. from my experience maybe 
1-5% of the lines are interleafed without the patch, too.


https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

No nothing. I think it is barely noticable in general. From prior long
runs i notices the scrambled messages, but it was only a small fraction.
So most of the time the threads dont seem to interfere, which makes the
lock kinda low-cost.

Am 07.08.2018 um 19:17 schrieb Andi via Phabricator:

> Abpostelnicu added a comment.
> 
> Did you notice any significant speed degradation?
> 
> https://reviews.llvm.org/D49851


https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:37
+void DurationDivisionCheck::check(const MatchFinder::MatchResult& result) {
+  const auto* op = result.Nodes.getNodeAs("op");
+  diag(op->getOperatorLoc(),

JonasToth wrote:
> Please follow the naming convention here as well -> `Op` or better `OpCall` 
> or similar to have more telling names.
`op` still is lowercase.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:30
+  hasOverloadedOperatorName("/"), argumentCountIs(2),
+  hasArgument(0, expr(is_duration)),
+  hasArgument(1, expr(is_duration)), expr().bind("OpCall",

s/is_duration/IsDuration/ twice



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

Please add one empty line above and please remove the `The improvements are...` 
line.

This might clash with other checks that are in review right now, but yours 
might be the first one that lands.


https://reviews.llvm.org/D50389



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

For reference, here is the output for llvm/lib.
F6896569: const_correctness_2_llvm_lib_references 


F6896567: const_correctness_2_llvm_lib_values_pointers 


Things i noticed:

- lambdas are warned as potential const, i think they should be excluded for 
the values
- for-loops that initialize two values (usually the start and end-iterator) are 
correctly diagnosed, but the diagnosis might be misleading. Especially because 
there is no way to have a const-iterator initialized together with the changing 
iterator.

`for (auto begin = vec.begin(), end = vec.end(); ...)`

- there seems to be a false positive with array-to-pointer decay. 
ExprMutAnalyzer does think of it, but maybe there is a bug in it.
- pointer diagnostics don't seem to worker (pointer-as-value). The test-case 
does work, This must be analyzed further
- there was a bug in the `check` method, where `AnalyzeValues == 0` did imply 
no analysis is done.

Given the volume of diagnostics for the value-case i did not investigate many 
cases manually. I think it makes sense to strive for code-transformation and 
check if the code still compiles.

References on the other hand seem to function correctly, and most references 
are single-var-decls, too. That means code transformation is very simple and 
therefor an easy target to check functionality.
References and values are in general treated the same, so it might give a hint 
about the performance for values.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Could you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
variable 'num' of type 'char [FloatData::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
  char num[FloatData::max_demangled_size] = {0};
 ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData::spec, value);
 ^


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.




Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:39
+  auto WhitelistClassMatcher =
+  cxxRecordDecl(hasAnyName(SmallVector(
+  WhitelistClasses.begin(), WhitelistClasses.end(;

I have seens this pattern now multiple times in various checks, we should have 
some utility wrapping the `hasAnyName(MyList)`. (Just in general, does not need 
to happen with this check)


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.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/performance-for-range-copy.cpp:1
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11 
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t 
-config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses", 
value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
 

I would prefer a single file, that has the configuration and leave the standard 
test like it is.

with this, you can always track what is actually done by default and what is 
done with different conigurations + potential inconsistencies that might occur 
by bugs in the configured code.


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.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Is the type important, or specifics about the variable (e.g. the name?)
The `_` variable names are sometimes used for RAII variables/lambdas
that shall just do some cleanup, e.g. gsl::finally().

It might make sense to give `ExprMutAnalyzer` an ignore-mechanism.

I wonder if instead there should be an option to not complain about the 
variables that aren't actually used?

Checking for variable usage would be simple. The `ExprMutAnalyzer`
always analyses the scope, e.g. a function. You can match this scope for
a `DeclRefExpr` of the variable, empty result means no usage.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@alexfh or @aaron.ballman could you take a final look on this one?


https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Given it is about the performance in loops and the optimizer?! can
better work with the copy-and-don't-use it might make sense to just
check if the variable is dereferenced in the scope of the loop (any
declRefExpr exists).

That is more userfriendly, too.

Am 08.08.2018 um 17:02 schrieb Haojian Wu via Phabricator:

> hokein added a comment.
> 
>> 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?
> 
> Yeah, it is google benchmark library.
> 
> The new fix `for (const auto& _ : state)` will trigger -Wunused warning. 
> `__attribute__((unused))` doesn't help to suppress the warning :(
> 
>   $ cat /tmp/main4.cc 
>   #include 
>   
>   struct __attribute__((unused)) Foo {
>   };
>   
>   void f() {
> std::vector foos;
> for (const Foo& _ : foos) {
> }
>   }
>   $ g++ /tmp/main4.cc -Wunused
>   
>
>   /tmp/main4.cc: In function ‘void f()’:
>   /tmp/main4.cc:8:19: warning: unused variable ‘_’ [-Wunused-variable]
>  for (const Foo& _ : foos) {
>  ^
> 
>> Is the type important, or specifics about the variable (e.g. the name?)
>> 
>>   The _ variable names are sometimes used for RAII variables/lambdas
>>   that shall just do some cleanup, e.g. gsl::finally().
>> 
>> It might make sense to give ExprMutAnalyzer an ignore-mechanism.
>> 
>> I wonder if instead there should be an option to not complain about the 
>> variables that aren't actually used?
>> 
>> Checking for variable usage would be simple. The ExprMutAnalyzer
>> 
>>   always analyses the scope, e.g. a function. You can match this scope for
>>   a DeclRefExpr of the variable, empty result means no usage.
> 
> Both type and variable name "_" can fix the issue here, but the variable name 
> seems a fragile way (if the name is changed, things will be broken again).
> 
> Yeah, adding an ignore mechanism to ExprMutAnalyzer is another option.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50447


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.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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

I see.




Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:93
 return false;
   if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
   .isMutated(&LoopVar))

Adding a `..IsMutated(&LoopVar) && IsReferenced(ForScope, LoopVar))` here 
should fix the warning as well.


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] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 159775.
JonasToth added a comment.

- fix bug with AnalyzeValues==false skipping whole check, adjust test code to 
'const' instead of const


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,557 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = &np_local0;
+  int *const p1_np_local0 = &np_local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = &np_local1;
+  int *const p1_np_local1 = &np_local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(&np_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = &np_local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared 'const'
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int &r0_np_local0 = np_local0;
+  int &r1_np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  double np_local0 = 24.4;
+
+  return &np_local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  //

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM, but alex or aaron should allow the commit ;)


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] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+  Finder->addMatcher(
+  nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(

Actually that one is generally useful. Accessing the `foo::internal` from 
outside of `foo` is always a problem. Maybe this matcher can become 
configurable or just match on any `internal` access from outside the enclosing 
namespace.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.h:21
+/// against doing so.
+/// Should not run on internal Abseil files or Abseil source code.
+///

Please make this a full sentence, like `This check should not be run on 
internal ...` (if that is grammatically correct)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50542



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Do you think it is a bad idea? If the variable is not used it is ok to
ignore it in this particular circumstance. Other warnings/check should
deal with such a situation IMHO.

Am 10.08.2018 um 10:29 schrieb Roman Lebedev via Phabricator:

> 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


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] D50389: [clang-tidy] new check for Abseil

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:32
+  hasImplicitDestinationType(qualType(unless(isInteger(,
+  unless(hasParent(cxxStaticCastExpr(,
+  this);

deannagarcia wrote:
> JonasToth wrote:
> > What about different kinds of casts, like C-Style casts?
> > Doesn't the `hasImplicitDestinationType` already remove the possibility for 
> > an cast as destination?
> I know the test fails without this line and flags the casts, so I'm pretty 
> sure it's necessary but I'm not exactly sure why hasImplicitDestinationType 
> doesn't catch it.
Ok. I can not help there. Just leave as is :)



Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:8
+division of two `absl::Duration` objects returns an `int64` with any fractional
+component truncated toward 0.
+

deannagarcia wrote:
> JonasToth wrote:
> > Please add one more sentence, why this is something you don't want, so it 
> > gets clear that floating point contextes are the interesting here.
> Does this link work or do you still want more?
Link is enough!


https://reviews.llvm.org/D50389



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:29
+  hasSourceExpression(ignoringParenCasts(cxxOperatorCallExpr(
+  hasOverloadedOperatorName("/"), argumentCountIs(2),
+  hasArgument(0, expr(IsDuration)),

The `argumentCountIs` should be redundant, not? Operator/ has always two 
operands and must be overloaded as an binary operator.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:31
+  hasArgument(0, expr(IsDuration)),
+  hasArgument(1, expr(IsDuration)), expr().bind("OpCall",
+  hasImplicitDestinationType(qualType(unless(isInteger(,

I dont understand the `expr().bind`, you can directly bind to the 
cxxOperatorCallExpr. That should be equivalent.



Comment at: clang-tidy/abseil/DurationDivisionCheck.h:19
+
+// Find potential incorrect uses of integer division of absl::Duration objects.
+class DurationDivisionCheck : public ClangTidyCheck {

The common `For the user-facing documentation see: ` is missing here.
All clang-tidy checks do have this (as it is autogenerated by the add-check 
tool) so i think it would be better to stay consistent with it.


https://reviews.llvm.org/D50389



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D36892



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM with only the formatting question.

But don't commit before any of the reviewers accepts (@alexfh @aaron.ballman 
usually have the last word)




Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:50
+ *result.SourceManager, result.Context->getLangOpts()),
+ ")");
+}

This line looks odd, does it come from clang-format?


https://reviews.llvm.org/D50389



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:50
+ *result.SourceManager, result.Context->getLangOpts()),
+ ")");
+}

deannagarcia wrote:
> JonasToth wrote:
> > This line looks odd, does it come from clang-format?
> I have run clang-format on this file and it's still formatted like this.
Leave as is :)


https://reviews.llvm.org/D50389



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+  Finder->addMatcher(
+  nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(

hugoeg wrote:
> JonasToth wrote:
> > Actually that one is generally useful. Accessing the `foo::internal` from 
> > outside of `foo` is always a problem. Maybe this matcher can become 
> > configurable or just match on any `internal` access from outside the 
> > enclosing namespace.
> That's a good idea. While we agree, right now our efforts are focused on 
> releasing abseil specific functions. Perhaps we can refactor this check at a 
> later time. 
Ok. Could you please add a `TODO` or `FIXME` that it is not forgotten?


https://reviews.llvm.org/D50542



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you for your first contribution to LLVM btw :)




Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+  TODO(hugoeg): refactor matcher to be configurable or just match on any 
internal access from outside the enclosing namespace.
+  

Missing //



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:35
+void NoInternalDepsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *intr_dep =
+  Result.Nodes.getNodeAs("internal_dep");

Please follow the LLVM naming convention (s/initr_dep/InternalDependency/ or 
similar)

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
for reference


https://reviews.llvm.org/D50542



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

No concerns except the small nits from my side.




Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+  // TODO(hugoeg): refactor matcher to be configurable or just match on any 
internal access from outside the enclosing namespace.
+  

Nit: This comment is very long, pls break the line



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:6
+
+Gives a warning if code using Abseil depends on internal details. If something 
is in a namespace or filename/path that includes the word “internal”, code is 
not allowed to depend upon it beaucse it’s an implementation detail. They 
cannot friend it, include it, you mention it or refer to it in any way. Doing 
so violtaes Abseil's compatibility guidelines and may result in breakage.
+

s/violtaes/violates/



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:8
+
+The folowing cases will result in warnings:
+

s/folowing/following/


https://reviews.llvm.org/D50542



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because
that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Am 10.08.2018 um 22:12 schrieb Shuai Wang via Phabricator:

> shuaiwang added a comment.
> 
> In https://reviews.llvm.org/D45444#1191874, @JonasToth wrote:
> 
>>> lCould you give a concrete example of this?
>> 
>> vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762
>> 
>> /home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
>> 
>>   variable 'num' of type 'char [FloatData::max_demangled_size]' can
>>   be declared 'const' [cppcoreguidelines-const-correctness]
>>     char num[FloatData::max_demangled_size] = {0};
>>    ^
>>   /home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
>>   variable 'n' of type 'int' can be declared 'const'
>>   [cppcoreguidelines-const-correctness]
>>         int n = snprintf(num, sizeof(num), FloatData::spec, value);
>>    ^
> 
> Looks like related to template.
>  the exact type of `num` depends on `Float`
>  and the `CallExpr` is not even resolved because we don't know the type of 
> the arguments yet (adl, overload resolution, etc.)
>  So the AST doesn't contain any array to pointer decay.
> 
> I guess we should suppress the check in such cases.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D45444


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could there even be a false positive? In a sense, the variable is never
used, so it does not matter, not?

Am 10.08.2018 um 22:17 schrieb Shuai Wang via Phabricator:

> shuaiwang added a comment.
> 
> In https://reviews.llvm.org/D50447#1194967, @JonasToth wrote:
> 
>> Do you think it is a bad idea? If the variable is not used it is ok to
>> 
>>   ignore it in this particular circumstance. Other warnings/check should
>>   deal with such a situation IMHO.
>> 
>> Am 10.08.2018 um 10:29 schrieb Roman Lebedev via Phabricator:
>> 
>>> 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
> 
> Not sure whether hokein have done that already but I did run through our code 
> base and AFAICT there's no false negative.
> 
> Repository:
> 
>   rL LLVM
> 
> https://reviews.llvm.org/D50447


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] D50580: [clang-tidy] Abseil: no namespace check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could it happen that some template specializations or so need to land in `absl`?




Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:28
+void NoNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *decl = Result.Nodes.getNodeAs("absl_namespace");
+

Please follow LLVM naming convention (Capitalize, and Decl should crash with a 
type)


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] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 160239.
JonasToth added a comment.

- explicitly ignore lambdas in VarDecls


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,563 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = &np_local0;
+  int *const p1_np_local0 = &np_local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = &np_local1;
+  int *const p1_np_local1 = &np_local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(&np_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = &np_local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared 'const'
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int &r0_np_local0 = np_local0;
+  int &r1_np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be de

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 160242.
JonasToth added a comment.

- update to current master of clang introduce CHECK-NOTES
- use new BeginLoc api


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

Files:
  clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -35,6 +36,7 @@
 
   try {
 throw non_derived_exception();
+
 // 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) {
@@ -50,24 +52,24 @@
   try {
 throw bad_inheritance();
 // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
-// CHECK MESSAGES: 10:1: note: type defined here
+// CHECK MESSAGES: 11:1: note: type defined here
 throw no_good_inheritance();
 // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
-// CHECK MESSAGES: 11:1: note: type defined here
+// CHECK MESSAGES: 12:1: note: type defined here
 throw really_creative();
 // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
-// CHECK MESSAGES: 12:1: note: type defined here
+// CHECK MESSAGES: 13:1: note: type defined here
   } catch (...) {
   }
   throw bad_inheritance();
   // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
-  // CHECK MESSAGES: 10:1: note: type defined here
+  // CHECK MESSAGES: 11:1: note: type defined here
   throw no_good_inheritance();
   // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
-  // CHECK MESSAGES: 11:1: note: type defined here
+  // CHECK MESSAGES: 12:1: note: type defined here
   throw really_creative();
   // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
-  // CHECK MESSAGES: 12:1: note: type defined here
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +93,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -128,7 +130,7 @@
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   THROW_EXCEPTION(non_derived_exception);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK MESSAGES: 9:1: note: type defined here
+  // CHECK MESSAGES: 10:1: note: type defined here
   THROW_EXCEPTION(std::exception);// Ok
   THROW_EXCEPTION(derived_exception); // Ok
   THROW_EXCEPTION(deep_hierarchy);// Ok
@@ -180,3 +182,21 @@
   // CHECK-NOTES: 169:1: note: type defined here
   throw UsingGood(); // Ok
 }
+
+// Fix PR37913
+struct invalid_argument_maker {
+  ::std::invalid_argument operator()() const;
+};
+struct int_maker {
+  int operator()() const;
+};
+template 
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+void exception_created_with_function() {
+  templated_thrower();
+  templated_thrower();
+
+  throw invalid_argument_maker{}();
+}
Index: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
===
--- clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
+++ clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
@@ -22,24 +22,39 @@
 return;
 
   Finder->addMatcher(
-  cxxThrowExpr(allOf(has(expr(unless(hasType(qualType(hasCanonicalType(
- hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(
- hasName("std::exception")),
- has(expr(unless(cxxUnresolvedConstructExpr(,
- eachOf(has(expr(hasType(namedDecl().bind("decl",
-anything(
+  cxxThrowExpr(
+  allOf(
+  ha

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you very much :)

Am 12.08.2018 um 00:17 schrieb Shuai Wang via Phabricator:

> shuaiwang added a comment.
> 
> In https://reviews.llvm.org/D45444#1196271, @JonasToth wrote:
> 
>> Always the same with the templates ;) So uninstantiated templates should
>> 
>>   just be ignored.
>> 
>> I think it would be better to have it in the ExprMutAnalyzer, because
>> 
>>   that part can not decide on const-ness. Fixing it here would just
>>   circumvent the problem but not fix it, would you agree?
> 
> Agreed :)
>  I'll create a diff handling various template related cases in 
> ExprMutationAnalyzer.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D45444


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+

hugoeg wrote:
> hokein wrote:
> > nit: please make sure the code follow LLVM code style, even for test code :)
> what is this in reference too?
> Will the test still work if I wrap the CHECK MESSAGE lines?
CHECK-MESSAGE can be on one line, even if its longer (that is common in the 
clang-tidy tests).

But dont use many empty lines and respect naming conventions and run 
clang-format over the code (except there is a valid reason that the formatting 
would infer with the tested logic).



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:11
+
+namespace absl {
+std::string StringsFunction (std::string s1){

hugoeg wrote:
> hokein wrote:
> > Since we have multiple abseil checks that might use these fake abseil 
> > declarations, I'd suggest pull out these to a common header, and include it 
> > in this test file.
> do I just put the header file  in test/clang-tidy ?
yes, there is one other similar case `hicpp-signed-bitwise-standard-types.h`


https://reviews.llvm.org/D50542



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.h:21
+/// against doing so. This check should not be run on internal Abseil files or
+///Abseil source code.
+///

double blank



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:17
+
+absl::strings_internal::foo();
+class foo{

That codeblock needs to be indented, see other checks for reference


https://reviews.llvm.org/D50542



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please add a test for the same usecase as in Sema.

  template 
  struct SizeIndicator {
constexpr int value = 8;
  };
  template <>
  struct SizeIndicator {
constexpr int value = 4;
  };
  template 
  void fooFunction() {
char Characters[SizeIndicator::value];
void ArrayToPointerDecay(Characters);
  }
  
  template <>
  void fooFunction() {
char Character[SizeIndicator::value];
void ArrayToPointerDecay(Characters);
  }

In both cases the mutation must be detected. I assume the function 
specialization is diagnosed correctly, because everything is resolved.




Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:454
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.y.z; }");

Is there already a test for a method from a templated type?

E.g.
```
template 
struct Foo {
  T x;
  Foo() { int local_int; x(local_int); }
};
```
One can not say that `local_int` could be const or not.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:454
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.y.z; }");

JonasToth wrote:
> Is there already a test for a method from a templated type?
> 
> E.g.
> ```
> template 
> struct Foo {
>   T x;
>   Foo() { int local_int; x(local_int); }
> };
> ```
> One can not say that `local_int` could be const or not.
`x.method(local_int);` would be interesting, just for security, given that the 
first case is an operator overload and the second one a classical method call.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+

hugoeg wrote:
> JonasToth wrote:
> > hugoeg wrote:
> > > hokein wrote:
> > > > nit: please make sure the code follow LLVM code style, even for test 
> > > > code :)
> > > what is this in reference too?
> > > Will the test still work if I wrap the CHECK MESSAGE lines?
> > CHECK-MESSAGE can be on one line, even if its longer (that is common in the 
> > clang-tidy tests).
> > 
> > But dont use many empty lines and respect naming conventions and run 
> > clang-format over the code (except there is a valid reason that the 
> > formatting would infer with the tested logic).
> Do my function names have to be verbs, they're not doing anything.
> 
> I could rename InternalFunction to something like InternallyProcessString 
> annd StringFunction to process String
> Would this be preferred?
It helps if you somehow show the "topic of the function". But I wrote some 
tests, that did not strictly follow and they passed review too ;)

Foo is just too generic, sth like `DirectAccess`, `FriendUsage`, 
`OpeningNamespace` or so is already telling and I guess good enough :)


https://reviews.llvm.org/D50542



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);

hugoeg wrote:
> deannagarcia wrote:
> > aaron.ballman wrote:
> > > hokein wrote:
> > > > aaron.ballman wrote:
> > > > > I think this needs a `not(isExpansionInSystemHeader())` in there, as 
> > > > > this is going to trigger on code that includes a header file using an 
> > > > > `absl` namespace. If I'm incorrect and users typically include abseil 
> > > > > as something other than system includes, you'll have to find a 
> > > > > different way to solve this.
> > > > Yeah, we have discussed this issue internally.  abseil-user code 
> > > > usually includes abseil header like `#include 
> > > > "whatever/abseil/base/optimization.h"`, so `IsExpansionInSystemHeader` 
> > > > doesn't work for most cases. 
> > > > 
> > > > We need a way to filter out all headers being a part of abseil library. 
> > > > I think we can create a matcher `InExpansionInAbseilHeader` -- 
> > > > basically using `IsExpansionInFileMatching` with a regex expression 
> > > > that matches typical abseil include path. What do you think?
> > > > 
> > > > And we'll have more abseil checks (e.g.  `abseil-no-internal-deps`) 
> > > > that use `InExpansionInAbseilHeader`, we should put it to a common 
> > > > header.
> > > I think that is a sensible approach.
> > We will begin working on this, I think it will first be implemented in 
> > abseil-no-internal-deps but once it looks good I will add it to this patch.
> I'm going to go ahead a implement this in ASTMatchers.h and include it on the 
> patch for **abseil-no-internal-dep**s
In principle it is ok, but I don't think ASTMatchers.h is the correct place, 
because it is only of use to clang-tidy.

There is a `util` directory in clang-tidy for this kind of stuff. Defining you 
own matchers works the same as in ASTMatchers, you can grep through clang-tidy 
checks (e.g. `AST_MATCHER`) to have some examples of private matchers.


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] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:5
+
+void DirectAcess(){
+  absl::strings_internal::InternalFunction();

Please run clang-format over the test code as well. The braces here in 
`FriendUsage` miss a space.


https://reviews.llvm.org/D50542



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

[PATCH] D50697: [clang-format] fix PR38557 - comments between "default" and ':' causes the case label to be treated as an identifier

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: hokein, krasimir, djasper, klimek.

The Bug was reported and fixed by Owen Pan.


Repository:
  rC Clang

https://reviews.llvm.org/D50697

Files:
  UnwrappedLineParser.cpp


Index: UnwrappedLineParser.cpp
===
--- UnwrappedLineParser.cpp
+++ UnwrappedLineParser.cpp
@@ -350,7 +350,10 @@
   break;
 case tok::kw_default: {
   unsigned StoredPosition = Tokens->getPosition();
-  FormatToken *Next = Tokens->getNextToken();
+  FormatToken *Next;
+  do {
+Next = Tokens->getNextToken();
+  } while (Next && Next->is(tok::comment));
   FormatTok = Tokens->setPosition(StoredPosition);
   if (Next && Next->isNot(tok::colon)) {
 // default not followed by ':' is not a case label; treat it like


Index: UnwrappedLineParser.cpp
===
--- UnwrappedLineParser.cpp
+++ UnwrappedLineParser.cpp
@@ -350,7 +350,10 @@
   break;
 case tok::kw_default: {
   unsigned StoredPosition = Tokens->getPosition();
-  FormatToken *Next = Tokens->getNextToken();
+  FormatToken *Next;
+  do {
+Next = Tokens->getNextToken();
+  } while (Next && Next->is(tok::comment));
   FormatTok = Tokens->setPosition(StoredPosition);
   if (Next && Next->isNot(tok::colon)) {
 // default not followed by ':' is not a case label; treat it like
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50697: [clang-format] fix PR38557 - comments between "default" and ':' causes the case label to be treated as an identifier

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 160550.
JonasToth added a comment.

Using git for the diff, add testcase


Repository:
  rC Clang

https://reviews.llvm.org/D50697

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1112,6 +1112,20 @@
"case 0: return; /* long long long long long long long long 
long long long long comment line */\n"
"}",
Style));
+
+  EXPECT_EQ("switch (n) {\n"
+"  default /*comments*/:\n"
+"return true;\n"
+"  case 0:\n"
+"return false;\n"
+"}",
+format("switch (n) {\n"
+   "  default /*comments*/:\n"
+   "return true;\n"
+   "case 0:\n"
+   "  return false;\n"
+   "}",
+   Style));
   verifyFormat("switch (a) {\n"
"#if FOO\n"
"case 0: return 0;\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -350,7 +350,10 @@
   break;
 case tok::kw_default: {
   unsigned StoredPosition = Tokens->getPosition();
-  FormatToken *Next = Tokens->getNextToken();
+  FormatToken *Next;
+  do {
+Next = Tokens->getNextToken();
+  } while (Next && Next->is(tok::comment));
   FormatTok = Tokens->setPosition(StoredPosition);
   if (Next && Next->isNot(tok::colon)) {
 // default not followed by ':' is not a case label; treat it like


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1112,6 +1112,20 @@
"case 0: return; /* long long long long long long long long long long long long comment line */\n"
"}",
Style));
+
+  EXPECT_EQ("switch (n) {\n"
+"  default /*comments*/:\n"
+"return true;\n"
+"  case 0:\n"
+"return false;\n"
+"}",
+format("switch (n) {\n"
+   "  default /*comments*/:\n"
+   "return true;\n"
+   "case 0:\n"
+   "  return false;\n"
+   "}",
+   Style));
   verifyFormat("switch (a) {\n"
"#if FOO\n"
"case 0: return 0;\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -350,7 +350,10 @@
   break;
 case tok::kw_default: {
   unsigned StoredPosition = Tokens->getPosition();
-  FormatToken *Next = Tokens->getNextToken();
+  FormatToken *Next;
+  do {
+Next = Tokens->getNextToken();
+  } while (Next && Next->is(tok::comment));
   FormatTok = Tokens->setPosition(StoredPosition);
   if (Next && Next->isNot(tok::colon)) {
 // default not followed by ':' is not a case label; treat it like
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50699: Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please create the diffs with full context (git diff -U 999) and add an 
test-case.


Repository:
  rC Clang

https://reviews.llvm.org/D50699



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

[PATCH] D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D50699#1198813, @owenpan wrote:

> Updated patch created by "svn diff --diff-cmd=diff -x -U99"


The patch seems not to be in the correct path (the `lib/Format` thing is 
missing). Could you check that again? If you plan to add more patches you could 
also have a look at `arc` which handles all of that for you :)


Repository:
  rC Clang

https://reviews.llvm.org/D50699



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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:191
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 
'int' is not derived from 'std::exception'
+

JonasToth wrote:
> JonasToth wrote:
> > alexfh wrote:
> > > hokein wrote:
> > > > I think giving message on the template function here is confusing to 
> > > > users even it gets instantiated somewhere in this TU -- because users 
> > > > have to find the location that triggers the template instantiation.
> > > > 
> > > > Maybe 
> > > > 1) Add a note which gives the instantiation location to the message, or
> > > > 2) ignore template case (some clang-tidy checks do this)
> > > In this particular case it seems to be useful to get warnings from 
> > > template instantiations. But the message will indeed be confusing.
> > > 
> > > Ideally, the message should have "in instantiation of xxx requested here" 
> > > notes attached, as clang warnings do. But this is not working 
> > > automatically, and it's implemented in Sema 
> > > (`Sema::PrintInstantiationStack()` in 
> > > lib/Sema/SemaTemplateInstantiate.cpp).
> > > 
> > > I wonder whether it's feasible to produce similar notes after Sema is 
> > > dead? Maybe not the whole instantiation stack, but at least it should be 
> > > possible to figure out that the enclosing function is a template 
> > > instantiation or is a member function of an type that is an instantiation 
> > > of a template. That would be useful for other checks as well.
> > It should be possible to figure out, that the type comes from template 
> > instantiation and that information could be added to the warning.
> > 
> > I will take a look at Sema and think about something like this. 
> > Unfortunatly i dont have a lot of time :/
> I did look further into the issue, i think it is non-trivial.
> 
> The newly added case is not a templated exception perse, but there is a 
> exception-factory, which is templated, that creates a normal exception.
> 
> I did add another note for template instantiations, but i could not figure 
> out a way to give better diagnostics for the new use-case.
@hokein and @alexfh Do you still have your concerns (the exception is not a 
template value, but the factory creating them) or is this fix acceptable?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:117
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(

I think you could add another test with `X x` (if you don't have it already 
and I overlooked them)



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:309
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =

I think we are missing tests for non-type template paramters (`template `). They should behave the same. But the following case would not be a 
mutation:

```
void non_mutating(const char* Array, int size) { /* Foo */ }
template 
struct ArrayLike {
  char* data[N]; // Initialized to something
  void SomeFunction() {
non_mutating(data, N);
  }
};
```

The difference between the 'normal' and non-type templates would be, that `N` 
is not mutatable at all and the semantics is clear (builtin integer-like type).

If the current implementation would not figure that out, you can just add a 
test for it and assume a mutation. Handling non-type templates later is 
absolutly ok.



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:410
+  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
+}

Out of curiosity: Why is the result with `y` different from the result for `x`? 
Both time `x` is mutated and `g()` mutates them.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I see, thank you for the clarification :)

Am 15.08.2018 um 19:25 schrieb Shuai Wang via Phabricator:

> shuaiwang added inline comments.
> 
> 
>  Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:410
>  +  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
>  +  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
>  +}
> 
>  
> 
> JonasToth wrote:
> 
>> Out of curiosity: Why is the result with `y` different from the result for 
>> `x`? Both time `x` is mutated and `g()` mutates them.
> 
> This is ultimately caused by not handling pointers yet.
>  As soon as the address of an object is taken we assume the object is mutated.
>  e.g.
> 
>   void f(const int*);
>   void g() {
> int x;
> f(&x); // <-- address of x taken, assume mutation
> int y[10];
> f(y); // <-- address of y taken, assume mutation
>   }
> 
> 
> And in all such cases the "mutated by" expression is the expression that 
> takes the address.
> 
> So back in this case, `g(x)` mutates `x` because we're assuming `g` mutates 
> its argument through non-const reference. Note that the declared `g` might 
> not be the one actually being called because of overload resolution, there 
> could be another `void g(char(&)[8])`
>  While for `g(y)` we know it's calling the `void g(char*)` so there's an 
> array to pointer decay, and the decay is the point we assumed mutation not 
> the function call.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50619


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

You are totally right.

Am 16.08.2018 um 02:41 schrieb Shuai Wang via Phabricator:

> shuaiwang added inline comments.
> 
> 
>  Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:309
> 
> +TEST(ExprMutationAnalyzerTest, CallUnresolved) {
>  +  auto AST =
> 
>  
> 
> JonasToth wrote:
> 
>> I think we are missing tests for non-type template paramters (`template 
>> `). They should behave the same. But the following case would not 
>> be a mutation:
>> 
>>   void non_mutating(const char* Array, int size) { /* Foo */ }
>>   template 
>>   struct ArrayLike {
>> char* data[N]; // Initialized to something
>> void SomeFunction() {
>>   non_mutating(data, N);
>> }
>>   };
>> 
>> 
>> The difference between the 'normal' and non-type templates would be, that 
>> `N` is not mutatable at all and the semantics is clear (builtin integer-like 
>> type).
>> 
>> If the current implementation would not figure that out, you can just add a 
>> test for it and assume a mutation. Handling non-type templates later is 
>> absolutly ok.
> 
> We have to assume `data` is mutated here as well. I'll add a test case for 
> this.
> 
>   void g(const char*, int); // <-- doesn't mutate
>   void g(char(&)[8], int); // <-- do mutate
>   
>   template 
>   void f() {
>   char data[N];
>   g(data, N); // <-- we don't know which `g` will be called yet
>   }
>   
>   void h() {
>   f<8>(); // <-- f calls g(char(&)[8], int) internally
>   f<9>(); // <-- f calls g(const char*, int) internally
>   }
> 
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50619


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@shuaiwang i tried to apply this and check the clang-tidy part again, but it 
does not compile (log attached).
I update clang to master, did you add a matcher or something like this?

F6950472: error.log 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

That sounds good as well, just not in clang and best in `clang-tidy/utils`

> `ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. 
> We  have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific 
> matchers. I'm not sure whether it is reasonable to put abseil-specific 
> matchers there. Maybe we create a `AbseilMatcher.h` file in 
> `clang-tidy/abseil` directory?
> 
> https://reviews.llvm.org/D50580


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] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:191
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 
'int' is not derived from 'std::exception'
+

hokein wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > JonasToth wrote:
> > > > alexfh wrote:
> > > > > hokein wrote:
> > > > > > I think giving message on the template function here is confusing 
> > > > > > to users even it gets instantiated somewhere in this TU -- because 
> > > > > > users have to find the location that triggers the template 
> > > > > > instantiation.
> > > > > > 
> > > > > > Maybe 
> > > > > > 1) Add a note which gives the instantiation location to the 
> > > > > > message, or
> > > > > > 2) ignore template case (some clang-tidy checks do this)
> > > > > In this particular case it seems to be useful to get warnings from 
> > > > > template instantiations. But the message will indeed be confusing.
> > > > > 
> > > > > Ideally, the message should have "in instantiation of xxx requested 
> > > > > here" notes attached, as clang warnings do. But this is not working 
> > > > > automatically, and it's implemented in Sema 
> > > > > (`Sema::PrintInstantiationStack()` in 
> > > > > lib/Sema/SemaTemplateInstantiate.cpp).
> > > > > 
> > > > > I wonder whether it's feasible to produce similar notes after Sema is 
> > > > > dead? Maybe not the whole instantiation stack, but at least it should 
> > > > > be possible to figure out that the enclosing function is a template 
> > > > > instantiation or is a member function of an type that is an 
> > > > > instantiation of a template. That would be useful for other checks as 
> > > > > well.
> > > > It should be possible to figure out, that the type comes from template 
> > > > instantiation and that information could be added to the warning.
> > > > 
> > > > I will take a look at Sema and think about something like this. 
> > > > Unfortunatly i dont have a lot of time :/
> > > I did look further into the issue, i think it is non-trivial.
> > > 
> > > The newly added case is not a templated exception perse, but there is a 
> > > exception-factory, which is templated, that creates a normal exception.
> > > 
> > > I did add another note for template instantiations, but i could not 
> > > figure out a way to give better diagnostics for the new use-case.
> > @hokein and @alexfh Do you still have your concerns (the exception is not a 
> > template value, but the factory creating them) or is this fix acceptable?
> I agree this is non-trivial. If we can't find a good solution at the moment, 
> I'd prefer to ignore this case instead of adding some workarounds in the 
> check, what do you think? 
Honestly I would let it as is. This test case is not well readable, but if we 
have something similar to

```
template 
void SomeComplextFunction() {
T ExceptionFactory;

   if (SomeCondition) 
 throw ExceptionFactory();
}
```
It is not that bad. And the check is still correct, just the code triggering 
this condition just hides whats happening.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714



___
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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.


I checked fast and `modernize-use-auto` does not catch this case.

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

I agree that this check is general for `make_...` templates. It should be 
either an addition to `modernize-use-auto` or `readability-` or so. What we do 
for such cases, where the check is generally useful and specific to a guideline 
at the same time is aliasing.

You can take a look at `hicpp-` module where most checks are actually aliases.


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] D50852: [clang-tidy] abseil-auto-make-unique

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



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

Please clang-format, `return` on next line.



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:23
+
+  using clang::ast_matchers::isTemplateInstantiation;
+  auto is_instantiation = decl(anyOf(cxxRecordDecl(isTemplateInstantiation()),

`clang::ast_matchers` is used already at line 14. No need to add this line.



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:27
+ functionDecl(isTemplateInstantiation(;
+  // There should be no additional expressions inbetween.
+  // E.g. this statement contains implicitCastExpr which makes it not eligible:

This sentence misses something. inbetween what?



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:34
+  has(ignoringParenImpCasts(callExpr(callee(functionDecl(
+  hasAnyName("absl::MakeUnique", "absl::make_unique",
+ "gtl::MakeUnique", "std::make_unique"),

The same rules apply for `make_shared`.

`make_pair` would be interesting as well, are there are standard `make_` 
templates?



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:51
+  var->getType()->getAsCXXRecordDecl());
+  if (!unique) return nullptr;
+  QualType type = unique->getTemplateArgs().get(0).getAsType();

Formatting and Naming conventions.

I think the following is more readable:

```
if (const auto* Unique = dyn_cast(var...)) {
  // stuff with var
}

return nullptr;
```



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:57
+const Type* GetMakeUniqueType(const FunctionDecl* make_unique_decl) {
+  const auto& template_arg =
+  
make_unique_decl->getTemplateSpecializationInfo()->TemplateArguments->get(

Naming conventions.



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:64
+void AutoMakeUniqueCheck::check(
+const ast_matchers::MatchFinder::MatchResult& result) {
+  const auto* var_decl = result.Nodes.getNodeAs("var_decl");

You don't need the `ast_matchers` namespace here.

Please follow the naming convention inside the function.



Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:69
+  if (var_decl->isOutOfLine()) {
+// "auto Struct::field = make_unique<...>();" doesn't work in GCC.
+return;

What exactly is the issue here in GCC? Please make this comment more 
explanatory and maybe add a `FIXME` to work around the issue (depending on the 
nature of the issue)



Comment at: docs/ReleaseNotes.rst:63
+
+  FIXME: add release notes.
+

Please add release notes.



Comment at: docs/clang-tidy/checks/abseil-auto-make-unique.rst:11
+  std::unique_ptr x = MakeUnique(...);
+
+with

Please add documentation that the 'Abstract Factory' use case is resolved 
correctly.



Comment at: test/clang-tidy/abseil-auto-make-unique.cpp:34
+void Primitive() {
+  std::unique_ptr x = absl::make_unique();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating 
the type name

How does the code look with multiple variable declarations?

Please add a test for
```
std::unique_ptr x = absl::make_unique(), y = 
absl::make_unique(), z;
```



Comment at: test/clang-tidy/abseil-auto-make-unique.cpp:73
+  // Different type. No change.
+  std::unique_ptr z = make_unique();
+  std::unique_ptr z2(make_unique());

lets consider a 3 level class hierarchy.

```
struct A { virtual void Something(); };
struct B : A { void Something() override; };
struct C : B { void Something() override; };

std::unique_ptr b_ptr = make_unique(), c_ptr = make_unique();
std::unique_ptr c_ptr2 = make_unique(), b_ptr2 = make_unique();
```

What is the behaviour? I expect that these places break when transformed. To 
avoid you can check the `VarDecl` `isSingleDecl()` (or similar, i forgot the 
exact name) and only emit a fixit if it is.
Doing type transformations for the multi-definitions is tricky.


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] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:24
+
+ast_matchers::internal::Matcher
+constructExprWithArg(llvm::StringRef ClassName,

you dont need the `ast_matchers` namespace as there is a `using namespace 
ast_matchers` at the top, same at other places.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:49
+  // in the character literal.
+  if (Result == R"("'")") {
+return std::string(R"('\'')");

The comment suggest, that all single quotes need to be escaped and then further 
processing happens, but you check on identity to `'` and return the escaped 
version of it. 
That confuses me.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:54
+  // Now replace the " with '.
+  auto Pos = Result.find_first_of('"');
+  if (Pos == Result.npos)

This expects at max 2 `"` characters. couldnt there be more?



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:71
+
+  // One character string literal.
+  const auto SingleChar =

Please make the comments full sentences



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:75
+
+  // string_view passed by value and contructed from string literal.
+  auto StringViewArg =

What about the `std::string_view`?



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:108
+
+  auto Replacement = makeCharacterLiteral(Literal);
+  if (!Replacement)

Please dont use auto here



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:118
+  diag(Literal->getBeginLoc(),
+   "absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal "
+   "consisting of a single character; consider using the more efficient "

You can configure this diagnostic with `%select{option1|option2}`.

See 
https://clang.llvm.org/docs/InternalsManual.html#formatting-a-diagnostic-argument
or `bugprone/ForwardingReferenceOverloadCheck.cpp` line 133 as an example 
(there are of course other places that technique is used)



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:119
+   "absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal "
+   "consisting of a single character; consider using the more efficient "
+   "overload accepting a character")

This diagnostic is really long, maybe you can shorten it a bit?

E.g. `consider using character overload` for the second part
and `.. called with single character string literal;`

My diagnostics are usually not so good, maybe you come up with something better 
:)



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.h:20
+/// Finds instances of absl::StrSplit() or absl::MaxSplits() where the 
delimiter
+/// is a single character string literal and replaces with a character.
+///

s/replaces with/replaces it with/



Comment at: docs/ReleaseNotes.rst:63
+
+  Finds instances of absl::StrSplit() or absl::MaxSplits() where the delimiter
+  is a single character string literal and replaces with a character.

Please highlight code with two `



Comment at: docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst:8
+where the delimiter is a single character string literal. The check will offer
+a suggestion to change the string literal into a character. It will also catch
+when code uses ``absl::ByAnyChar()`` for just a single character and will

I think:
It will also catch code using ...

sounds a little better



Comment at: test/clang-tidy/abseil-faster-strsplit-delimiter.cpp:42
+void SplitDelimiters() {
+  absl::StrSplit("ABC", "A");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: 
absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal 
consisting of a single character; consider using the more efficient overload 
accepting a character [abseil-faster-strsplit-delimiter]

Please add a test, where `"A"` is used as an arbitray function argument 
(similar to the template case, but without templates involved)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50862



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-auto-make-unique.cpp:73
+  // Different type. No change.
+  std::unique_ptr z = make_unique();
+  std::unique_ptr z2(make_unique());

JonasToth wrote:
> lets consider a 3 level class hierarchy.
> 
> ```
> struct A { virtual void Something(); };
> struct B : A { void Something() override; };
> struct C : B { void Something() override; };
> 
> std::unique_ptr b_ptr = make_unique(), c_ptr = make_unique();
> std::unique_ptr c_ptr2 = make_unique(), b_ptr2 = make_unique();
> ```
> 
> What is the behaviour? I expect that these places break when transformed. To 
> avoid you can check the `VarDecl` `isSingleDecl()` (or similar, i forgot the 
> exact name) and only emit a fixit if it is.
> Doing type transformations for the multi-definitions is tricky.
`isSingleDecl()` is a member of `DeclStmt`.


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] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D49851#1202764, @Abpostelnicu wrote:

> Strangely I haven't had those kind of issues but since you propose those 
> modifications I would do one more modification, let's output everything to 
> stdout and not stderr by doing:
>
>   if err:
> sys.stdout.write(str(err) + '\n')
>   


You can make this a new revision, fixing the `byte` and `str` issues would be 
more important now.

The `byte` and `str` thingie is since the whole python3 releases or did it 
change?


Repository:
  rL LLVM

https://reviews.llvm.org/D49851



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:21
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+  if (loc.isInvalid()) {
+return false;

You can elide the braces for single stmt ifs



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+  }
+  auto FileEntry =
+  manager.getFileEntryForID(manager.getFileID(loc));

Please dont use `auto` here as the typ is not clear



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:26
+  manager.getFileEntryForID(manager.getFileID(loc));
+  if (!FileEntry) {
+return false;

ellide braces



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:29
+  }
+  auto Filename = FileEntry->getName();
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"

no `auto` plz



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:32
+  "synchronization|types|utiliy)");
+  return (RE.match(Filename));
+}

Are the parens necessary? I think `match` does return a `bool`, then you can 
just return it without the braces. If not, please make it a boolean with an 
expression like `match() > 0` or so.


https://reviews.llvm.org/D50542



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

[PATCH] D50883: [clang-tidy] Handle unique owning smart pointers in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I am suprised that this does not automatically follow from the general rules. 
At the end, smartpointers cant do anything else then 'normal' classes.

The `operator+/->` were not handled before? The mutation of `SmartPtr x; 
x->mf();` should already be catched, not?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50883



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

[PATCH] D50883: [clang-tidy] Handle unique owning smart pointers in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I see, thank you :)

Am 17.08.2018 um 18:55 schrieb Shuai Wang via Phabricator:

> shuaiwang added a comment.
> 
> In https://reviews.llvm.org/D50883#1203690, @JonasToth wrote:
> 
>> I am suprised that this does not automatically follow from the general 
>> rules. At the end, smartpointers cant do anything else then 'normal' classes.
>> 
>> The `operator+/->` were not handled before? The mutation of `SmartPtr x; 
>> x->mf();` should already be catched, not?
> 
> Different from `std::vector::operator[]` which has two overloads for const 
> and non-const access, `std::unique_ptr` only has one const version of 
> `operator->`.
>  So for `SmartPtr x; x->mf();` we only see a const operator being invoked on 
> `x`. `mf` is not a member of `SmartPtr` and the member call to `mf` is not on 
> `x` directly, we never followed that far.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50883


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50883



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:85
+
+  // absl::StrSplit(..., "x") ->  absl::StrSplit(..., 'x')
+  // absl::ByAnyChar("x") -> 'x'

Maybe you could make these comments into full sentences. I do think its clear 
what they mean, but the rule is full sentences.

Maybe `Find uses of 'absl::...' to transform them into 'absl::...'.`?



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:115
+
+  if (const auto *ByAnyChar = Result.Nodes.getNodeAs("ByAnyChar")) {
+Range = ByAnyChar->getSourceRange();

You can ellide the braces in this case.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:49
+  // in the character literal.
+  if (Result == R"("'")") {
+return std::string(R"('\'')");

deannagarcia wrote:
> JonasToth wrote:
> > The comment suggest, that all single quotes need to be escaped and then 
> > further processing happens, but you check on identity to `'` and return the 
> > escaped version of it. 
> > That confuses me.
> Does the new comment clear it up?
Yes, thank you.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:54
+  // Now replace the " with '.
+  auto Pos = Result.find_first_of('"');
+  if (Pos == Result.npos)

deannagarcia wrote:
> JonasToth wrote:
> > This expects at max 2 `"` characters. couldnt there be more?
> The only way this will be run is if the string is a single character, so the 
> only possibility if there are more than 2 " characters is that the character 
> that is the delimiter is actually " which is taken care of in the check. Does 
> that make sense/do you think I need to add a comment about this?
Ok I see, you could add an assertion before this section. Having the 
precondition checked is always a good thing and usually adds clarity as well :)



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:75
+
+  // string_view passed by value and contructed from string literal.
+  auto StringViewArg =

deannagarcia wrote:
> JonasToth wrote:
> > What about the `std::string_view`?
> This matcher is only here for the next matcher to check to see if 
> absl::ByAnyChar has been passed in a single character string view and is only 
> necessary because ByAnyChar accepts an absl::string_view so it isn't 
> necessary to make one for std::string_view
Ok.



Comment at: test/clang-tidy/abseil-faster-strsplit-delimiter.cpp:42
+void SplitDelimiters() {
+  absl::StrSplit("ABC", "A");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: 
absl::StrSplit()/ByAnyChar()/MaxSplits() called with a string literal 
consisting of a single character; consider using the more efficient overload 
accepting a character [abseil-faster-strsplit-delimiter]

deannagarcia wrote:
> JonasToth wrote:
> > Please add a test, where `"A"` is used as an arbitray function argument 
> > (similar to the template case, but without templates involved)
> Can you explain this more/provide an example? 
The testcase i had in mind does not make sense, sorry for noise.


https://reviews.llvm.org/D50862



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:54
+  // Now replace the " with '.
+  auto Pos = Result.find_first_of('"');
+  if (Pos == Result.npos)

deannagarcia wrote:
> JonasToth wrote:
> > deannagarcia wrote:
> > > JonasToth wrote:
> > > > This expects at max 2 `"` characters. couldnt there be more?
> > > The only way this will be run is if the string is a single character, so 
> > > the only possibility if there are more than 2 " characters is that the 
> > > character that is the delimiter is actually " which is taken care of in 
> > > the check. Does that make sense/do you think I need to add a comment 
> > > about this?
> > Ok I see, you could add an assertion before this section. Having the 
> > precondition checked is always a good thing and usually adds clarity as 
> > well :)
> I can't think of a simple thing to assert because most literals will look 
> like: "a" but there is also the possibility that it looks like "\"" and I 
> can't think of an easy way to combine those two. Do you have an idea/maybe I 
> could just put a comment at the beginning saying this is only meant for 
> single character string literals?
Wouldnt that result in an assert approximatly this:

`assert(Result.size() == 1 || (Result.size() == 2 && Result.startswith('\\'))`

Stating the size constraint alone is already a valuable assert in my opinion.



Comment at: clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:85
+
+  // Find uses of absl::StrSplit(..., "x") and absl::StrSplit(..., 
absl::ByAnyChar("x"))
+  // to transform them into absl::StrSplit(..., 'x').

These comments seem to be longer then 80, if true please wrap them.


https://reviews.llvm.org/D50862



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D50542#1205880, @hugoeg wrote:

> Anymore changes I should make or issues I should be aware of?


From my side not!
You still have unresolved comments that cause the revision to not be `Ready To 
Review` for the main reviewers, maybe that causes them to overlook it.




Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+

JonasToth wrote:
> hugoeg wrote:
> > JonasToth wrote:
> > > hugoeg wrote:
> > > > hokein wrote:
> > > > > nit: please make sure the code follow LLVM code style, even for test 
> > > > > code :)
> > > > what is this in reference too?
> > > > Will the test still work if I wrap the CHECK MESSAGE lines?
> > > CHECK-MESSAGE can be on one line, even if its longer (that is common in 
> > > the clang-tidy tests).
> > > 
> > > But dont use many empty lines and respect naming conventions and run 
> > > clang-format over the code (except there is a valid reason that the 
> > > formatting would infer with the tested logic).
> > Do my function names have to be verbs, they're not doing anything.
> > 
> > I could rename InternalFunction to something like InternallyProcessString 
> > annd StringFunction to process String
> > Would this be preferred?
> It helps if you somehow show the "topic of the function". But I wrote some 
> tests, that did not strictly follow and they passed review too ;)
> 
> Foo is just too generic, sth like `DirectAccess`, `FriendUsage`, 
> `OpeningNamespace` or so is already telling and I guess good enough :)
I think the names are ok.


https://reviews.llvm.org/D50542



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

  → ~/opt/llvm/build/bin/clang-tidy 
-checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
  clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* 
clang::CXXDependentScopeMemberExpr::getBase() const: Assertion 
`!isImplicitAccess()' failed.
  Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Sure, thanks for the fast fix :)

Am 22.08.2018 um 00:04 schrieb Shuai Wang via Phabricator:

> shuaiwang added a comment.
> 
> In https://reviews.llvm.org/D50619#1207785, @JonasToth wrote:
> 
>> @shuaiwang Unfortunatly the analysis does not pass and fails on an assertion
>> 
>>   → ~/opt/llvm/build/bin/clang-tidy 
>> -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
>>   clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* 
>> clang::CXXDependentScopeMemberExpr::getBase() const: Assertion 
>> `!isImplicitAccess()' failed.
>>   Abgebrochen (Speicherabzug geschrieben)
>> 
>> I did not investigate further, sorry for the long time to try it out.
> 
> https://reviews.llvm.org/D50617 updated, could you help try again? Thanks!
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50619


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM


https://reviews.llvm.org/D50862



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

[PATCH] D50617: [ASTMatchers] Let hasObjectExpression also support UnresolvedMemberExpr, CXXDependentScopeMemberExpr

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

The fix was effective, the assertion does not fire anymore on the 
`ItaniumDemangle.cpp` file


Repository:
  rC Clang

https://reviews.llvm.org/D50617



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:28
+  for (;;) {
+if (const auto *MTE = dyn_cast(E)) {
+  E = MTE->getTemporary();

You can ellide the braces for the single stmt ifs



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:52
+
+  const std::string str_cat = "::absl::StrCat";
+  const std::string alpha_num = "::absl::AlphaNum";

The naming is ambigous. `str_cat` can easily be mixed with `strcat`. Is this 
constant even necessary or could you use the literal in the functionDecl 
matcher?



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:85
+
+  // x = StrCat(x) does nothing.
+  if (call->getNumArgs() == 1) {

Please make this comment a full sentence



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:87
+  if (call->getNumArgs() == 1) {
+diag(op->getBeginLoc(), "call to StrCat does nothing");
+return;

I think it would be better to include `absl::StrCat` in the diagnostic, as it 
is more clear.
Same opinion on the other diag.



Comment at: test/clang-tidy/abseil-str-cat-append.cpp:91
+
+void bar() {
+  std::string a, b;

What happens if `StrCat` is used e.g. in an `std::accumulate` call as the 
binary operator? (https://en.cppreference.com/w/cpp/algorithm/accumulate  the 
Example includes such a case)
Is this diagnosed, too?

The general case would be if `StrCat` is used as a template argument for a 
functor.



Comment at: test/clang-tidy/abseil-str-cat-append.cpp:114
+
+}  // namespace absl

Could you please add a testcase that is outside of the `absl` namespace.

```
void f() {
  std::string s;
  s = absl::StrCat(s,s);
}
```

and 
```
void g() {
  std::string s;
  using absl::StrCat;
  s = StrCat(s, s);
}
```

with better naming.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51061



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-str-cat-append.cpp:91
+
+void bar() {
+  std::string a, b;

hugoeg wrote:
> JonasToth wrote:
> > What happens if `StrCat` is used e.g. in an `std::accumulate` call as the 
> > binary operator? (https://en.cppreference.com/w/cpp/algorithm/accumulate  
> > the Example includes such a case)
> > Is this diagnosed, too?
> > 
> > The general case would be if `StrCat` is used as a template argument for a 
> > functor.
> it doesn't diagnose those cases. and diagnosing it doesn't seem to be 
> worthwhile
You could add it as a known limitation to the docs / or as TODO in the code.


https://reviews.llvm.org/D51061



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping. is there something obviously wrong with this check?


https://reviews.llvm.org/D37808



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

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:357
+  ConstExpr = Result.Nodes.getNodeAs(CstId);
+  return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);
+}

xazax.hun wrote:
> I think you could just return the pointer and return a null pointer in case 
> it is not an integerConstantExpr. This way no compatibility overload needed.
or `llvm::Optional` making it clear that no result is intended.


Repository:
  rL LLVM

https://reviews.llvm.org/D38688



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 118911.
JonasToth added a comment.

ping: I don't understand the lack of feedback. This check should not be a 
frontend warning,
since it warns for perfectly valid code. It is supposed to give stronger 
guarantees
for developers requiring this.

- rebased to master


https://reviews.llvm.org/D37808

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,406 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+/// All of these cases will not emit a warning per default, but with explicit activation.
+void problematic_if(int i, enum OS os) {
+  if (i > 0) {
+return;
+  } else if (i < 0) {
+return;
+  }
+
+  if (os == Mac) {
+return;
+  } else if (os == Linux) {
+if (true) {
+  return;
+} else if (false) {
+  return;
+}
+return;
+  } else {
+/* unreachable */
+if (true) // check if the parent would match here as well
+  return;
+  }
+
+  // Ok, because all paths are covered
+  if (i > 0) {
+return;
+  } else if (i < 0) {
+return;
+  } else {
+/* error, maybe precondition failed */
+  }
+}
+
+int return_integer() { return 42; }
+
+void problematic_switch(int i) {
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerate switch statement without any cases found; add cases and a default branch
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default branch.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch stmt found; only the default branch exists
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default branch -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch case could be better written with single if stmt
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch
+  case 0:
+  case 1:
+break;
+  }
+
+  const int &IntRef = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  // The switch stmt covers all paths explicitly.
+  // FIXME: False positive, determine the count of different possible values correctly.
+  // On the other hand, char is not defined to be one byte big, so there could be
+  // architectural differences, which would make the 'default:' case more portable.
+  switch (c) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 82:
+  case 83:
+  case 84:
+  case 85:
+  case 86:
+  case 87:
+  case 88:
+  case 89:
+  ca

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:85
+diag(ElseIfWithoutElse->getLocStart(),
+ "potential uncovered codepath found; add an ending else branch");
+return;

JDevlieghere wrote:
> I'm not a big fan of the 'found', can we just omit it? The same goes for the 
> other diags. 
Agree. The messages are better without 'found'.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96
+// Only the default branch (we explicitly matched for default!) exists.
+if (CaseCount == 1) {
+  diag(SwitchWithDefault->getLocStart(),

JDevlieghere wrote:
> Why not a switch?
I intent to check if all cases are explicitly covered.

In the testcases there is one switch with all numbers explicitly written, 
meaning there is no need to add a default branch.

This would allow further 
```
else if (CaseCount == MaximumPossibleCases) { /* No warning */ }
```
path which is not modable with `switch`.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:152
+// Should be written as an IfStmt.
+if (CaseCount == 1) {
+  diag(SwitchWithoutDefault->getLocStart(), "switch stmt with only one "

JDevlieghere wrote:
> I'm aware that the message and fixme are different, but since the structure 
> is so similar to the handling of the other switch case, I wonder if there is 
> any chance we could extract the common parts?
I try to get something shorter.
Maybe 
```
if(CaseCount == 1 && MatchedSwitch) {}
else if(CaseCount == 1 && MatchedElseIf) {}
```
?


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 118951.
JonasToth marked 2 inline comments as done.
JonasToth added a comment.

- major codechange


https://reviews.llvm.org/D37808

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,445 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+struct Bitfields {
+  unsigned UInt : 3;
+  int SInt : 1;
+};
+
+int return_integer() { return 42; }
+
+void bad_switch(int i) {
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default case.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default case -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as if/else statement
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  const int &IntRef = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+
+  Bitfields Bf;
+  // UInt has 3 bits size.
+  switch (Bf.UInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.UInt) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+break;
+  }
+  // SInt has 1 bit size, so this is somewhat degenerated.
+  switch (Bf.SInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.SInt) {
+  case 0:
+  case 1:
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  switch (c) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 82:
+  case 83:
+  case 84:
+  case 85:
+  case 86:
+  case 87:
+  case 88:
+  case 89:
+  case 90:
+  case 91:
+  case 92:
+  case 93:
+  case 94:
+  case 95:
+  case 96:
+  case 97:
+  case 98:
+  case 99:
+  case 100:
+  case 101:
+  case 102:
+  case 103:
+  case 104:
+  case 105:
+  case 106:
+  case 107:
+  case 108:
+  case 109:
+  case 110:
+  case 111:
+  case 112:
+  case 113:
+  case 114:
+  case 115:
+  case 116:
+  case 117:
+  case 118:
+  case 119:
+  case 120:
+  case 121:
+  case 122:
+  case 123:
+  case 124:
+  case 125:
+  case 126:
+  case 127:
+  cas

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 6 inline comments as done.
JonasToth added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96
+// Only the default branch (we explicitly matched for default!) exists.
+if (CaseCount == 1) {
+  diag(SwitchWithDefault->getLocStart(),

JonasToth wrote:
> JDevlieghere wrote:
> > Why not a switch?
> I intent to check if all cases are explicitly covered.
> 
> In the testcases there is one switch with all numbers explicitly written, 
> meaning there is no need to add a default branch.
> 
> This would allow further 
> ```
> else if (CaseCount == MaximumPossibleCases) { /* No warning */ }
> ```
> path which is not modable with `switch`.
Woops. I mixed codeplaces (since bad duplication from my side). I did merge the 
diagnostics better, making this a natural fit for a switch.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:152
+// Should be written as an IfStmt.
+if (CaseCount == 1) {
+  diag(SwitchWithoutDefault->getLocStart(), "switch stmt with only one "

JDevlieghere wrote:
> JonasToth wrote:
> > JDevlieghere wrote:
> > > I'm aware that the message and fixme are different, but since the 
> > > structure is so similar to the handling of the other switch case, I 
> > > wonder if there is any chance we could extract the common parts?
> > I try to get something shorter.
> > Maybe 
> > ```
> > if(CaseCount == 1 && MatchedSwitch) {}
> > else if(CaseCount == 1 && MatchedElseIf) {}
> > ```
> > ?
> Wouldn't it be easier to have a function and pass as arguments the stuff 
> that's different? Both are `SwitchStmt`s so if you pass that + the diagnostic 
> message you should be able to share the other logic. 
I kinda rewrote the whole checking part. 
Updated diff comes in a sec. I found that bitfields are not handled as they 
should. 




https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 4 inline comments as done.
JonasToth added a comment.

Improved check a lot. Hope reviewers now have an easier time reading it.


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

F5426271: llvm_lib_target_x86_paths 
Example output for `llvm/lib/Target/X86`
Running it over the whole `llvm/lib` codebase generates a lot of warnings. 
Please note, that it seems to be common to write code like this:

  int Val;
  switch(Val) {
case 1: // something
case 2: // something else
case 16: // magic
  }
  llvm_unreachable("reason");

In some cases it has been taken care that no fallthrough happens, but it's not 
so simple to spot. Using `default: llvm_unreachable("reason");` would comply 
with the check.


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 4 inline comments as done.
JonasToth added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:98
+  // hold.
+  const std::size_t BitCount = [&T, &Context]() {
+if (T->isIntegralType(Context))

JDevlieghere wrote:
> Unless you expect a whole bunch of logic to be added here, I'd un-const and 
> initialize BitCount to zero, then just have if-clause reassign it and get rid 
> of the lambda. This will save you a few lines of code and complexity. 
I dont know if i missed some integral types, that need special care, so this 
one was defensive.

But with the refactoring i made, I should change this, agreed.


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 119440.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- address review comments


https://reviews.llvm.org/D37808

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,445 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+struct Bitfields {
+  unsigned UInt : 3;
+  int SInt : 1;
+};
+
+int return_integer() { return 42; }
+
+void bad_switch(int i) {
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default case.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default case -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as if/else statement
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  const int &IntRef = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+
+  Bitfields Bf;
+  // UInt has 3 bits size.
+  switch (Bf.UInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.UInt) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+break;
+  }
+  // SInt has 1 bit size, so this is somewhat degenerated.
+  switch (Bf.SInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.SInt) {
+  case 0:
+  case 1:
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  switch (c) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 82:
+  case 83:
+  case 84:
+  case 85:
+  case 86:
+  case 87:
+  case 88:
+  case 89:
+  case 90:
+  case 91:
+  case 92:
+  case 93:
+  case 94:
+  case 95:
+  case 96:
+  case 97:
+  case 98:
+  case 99:
+  case 100:
+  case 101:
+  case 102:
+  case 103:
+  case 104:
+  case 105:
+  case 106:
+  case 107:
+  case 108:
+  case 109:
+  case 110:
+  case 111:
+  case 112:
+  case 113:
+  case 114:
+  case 115:
+  case 116:
+  case 117:
+  case 118:
+  case 119:
+  case 120:
+  case 121:
+  case 122:
+  case 123:
+  case 124:
+  case 125:
+  case 126:
+  case 127:

[PATCH] D39027: [docs][refactor] Add a new tutorial that talks about how one can implement refactoring actions

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: docs/RefactoringActionTutorial.rst:90
+Let's call it something like ``FlattenIfStatements.cpp``. Don't forget to add
+it to the ``CMakeLists.txt`` file that's located in the
+``lib/Tooling/Refactoring`` directory.

Will there be functionality like `add-new-check.py` in clang-tidy? Maybe we 
could work on this to ease getting started.


Repository:
  rL LLVM

https://reviews.llvm.org/D39027



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 119553.
JonasToth added a comment.

- Improve docs, grammar


https://reviews.llvm.org/D37808

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,445 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+struct Bitfields {
+  unsigned UInt : 3;
+  int SInt : 1;
+};
+
+int return_integer() { return 42; }
+
+void bad_switch(int i) {
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default case.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default case -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as if/else statement
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  const int &IntRef = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+
+  Bitfields Bf;
+  // UInt has 3 bits size.
+  switch (Bf.UInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.UInt) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+break;
+  }
+  // SInt has 1 bit size, so this is somewhat degenerated.
+  switch (Bf.SInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.SInt) {
+  case 0:
+  case 1:
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  switch (c) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 82:
+  case 83:
+  case 84:
+  case 85:
+  case 86:
+  case 87:
+  case 88:
+  case 89:
+  case 90:
+  case 91:
+  case 92:
+  case 93:
+  case 94:
+  case 95:
+  case 96:
+  case 97:
+  case 98:
+  case 99:
+  case 100:
+  case 101:
+  case 102:
+  case 103:
+  case 104:
+  case 105:
+  case 106:
+  case 107:
+  case 108:
+  case 109:
+  case 110:
+  case 111:
+  case 112:
+  case 113:
+  case 114:
+  case 115:
+  case 116:
+  case 117:
+  case 118:
+  case 119:
+  case 120:
+  case 121:
+  case 122:
+  case 123:
+  case 124:
+  case 125:
+  case 126:
+  case 127:
+  case 128:
+  case 129:
+  case 130:
+  cas

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> I agree that we should not spend too much effort on making warnings from the 
> compiler and tidy disjunct.

+1
There is an effort to treat clangs frontend warnings similar to clang-tidy 
checks within clang-tidy. This would allow to manage the overlap as well.

Will this check find stuff like this (or is it part of the frontend)

  int throwing() {
  throw int(42);
  }
  
  int not_throwing() noexcept {
  throwing();
  }

It would be nice to have a check that could automatically introduce `noexcept` 
into a codebase for cases it safe. I think this path would be a good place for 
it.


https://reviews.llvm.org/D33537



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

simplified the if-else stuff


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 120251.
JonasToth marked 8 inline comments as done.
JonasToth added a comment.

- Merge 'master'
- address review comments


https://reviews.llvm.org/D37808

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,445 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+struct Bitfields {
+  unsigned UInt : 3;
+  int SInt : 1;
+};
+
+int return_integer() { return 42; }
+
+void bad_switch(int i) {
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default case.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default case -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as if/else statement
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  const int &IntRef = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+
+  Bitfields Bf;
+  // UInt has 3 bits size.
+  switch (Bf.UInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath; add a default case
+  case 0:
+  case 1:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.UInt) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+break;
+  }
+  // SInt has 1 bit size, so this is somewhat degenerated.
+  switch (Bf.SInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use if statement
+  case 0:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.SInt) {
+  case 0:
+  case 1:
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  switch (c) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 82:
+  case 83:
+  case 84:
+  case 85:
+  case 86:
+  case 87:
+  case 88:
+  case 89:
+  case 90:
+  case 91:
+  case 92:
+  case 93:
+  case 94:
+  case 95:
+  case 96:
+  case 97:
+  case 98:
+  case 99:
+  case 100:
+  case 101:
+  case 102:
+  case 103:
+  case 104:
+  case 105:
+  case 106:
+  case 107:
+  case 108:
+  case 109:
+  case 110:
+  case 111:
+  case 112:
+  case 113:
+  case 114:
+  case 115:
+  case 116:
+  case 117:
+  case 118:
+  case 119:
+  case 120:
+  case 121:
+  case 122:
+  case 123:
+  case 124:
+  case 125:
+  case 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:21
+namespace {
+
+// Skips any combination of temporary materialization, temporary binding and

I think you can elide the empty line around the matcher.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:49
+   return;
+  // Look for calls to StrCat.
+  const auto StrCat = functionDecl(hasName("::absl::StrCat"));

Not sure if this comment adds value and might even be a little misleading. 
`StrCat` itself does not look for calls, it is a helper to do so.
Maybe you can move the comment to the actual matching part with everything 
composed and explain more what is specifically looked for.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:51
+  const auto StrCat = functionDecl(hasName("::absl::StrCat"));
+  // The arguments of StrCat are implicitly converted to AlphaNum. Match that.
+  const auto AlphaNum = ignoringTemporaries(cxxConstructExpr(

Please make this comment a sentence. `Match that the arguments ...` would be ok 
i think.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:57
+
+  const auto has_another_reference_to_lhs =
+  callExpr(hasAnyArgument(expr(hasDescendant(declRefExpr(

Please rename this variable to follow the convention



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:79
+  const auto *Call = Result.Nodes.getNodeAs("Call");
+
+  // Handles the case where x = absl::StrCat(x), which does nothing.

please add an `assert(Op != nullptr && Call != nullptr && "Matcher does not 
work as expected")`, even though it seems trivial its better then nullptr deref.
And the logic might change in the future, so better be save



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:80
+
+  // Handles the case where x = absl::StrCat(x), which does nothing.
+  if (Call->getNumArgs() == 1) {

I think `  // Handles the case 'x = absl::StrCat(x)', which does nothing.` 
would be better



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:82
+  if (Call->getNumArgs() == 1) {
+diag(Op->getBeginLoc(), "call to absl::StrCat does nothing");
+return;

please use 'absl::StrCat' in the diagnostics (same below) to signify that it is 
code.


https://reviews.llvm.org/D51061



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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping @alexfh what is your take on the issue?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714



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

[PATCH] D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Owen, shall i commit for you?


Repository:
  rC Clang

https://reviews.llvm.org/D50699



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please dont work in this revision but create a new one (as this one is
already committed)!

Am 24.08.2018 um 17:34 schrieb Andi via Phabricator:

> Abpostelnicu updated this revision to Diff 162387.
> 
> https://reviews.llvm.org/D49851
> 
> Files:
> 
>   clang-tidy/tool/run-clang-tidy.py
> 
> Index: clang-tidy/tool/run-clang-tidy.py
>  ===
> 
>   - clang-tidy/tool/run-clang-tidy.py +++ clang-tidy/tool/run-clang-tidy.py 
> @@ -166,10 +166,18 @@ output, err = proc.communicate() if proc.returncode != 
> 0: failed_files.append(name) + +if is_py2: +  output_string = output 
> +  err_string = err +else: +  output_string = str(output, 
> 'utf-8') +  err_string = str(err, 'utf-8') + with lock:
> - sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
> - if err > 0:
> - sys.stderr.write(err + '\n') +  sys.stdout.write(' '.join(invocation) + 
> '\n' + output_string + '\n') +  if err: +
> sys.stderr.write(err_string + '\n') queue.task_done()


https://reviews.llvm.org/D49851



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please create the patch with full context 
(https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51220



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I don't know a lot about how to write portable code!

But wouldn't this be out usecase? 
http://python-future.org/compatible_idioms.html#file-io-with-open
Using the `decode` is supposed to work in both pythons


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51220



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could you please verify it is actually working?

Am 24.08.2018 um 18:05 schrieb Andi via Phabricator:

> Abpostelnicu added a comment.
> 
> In https://reviews.llvm.org/D51220#1212463, @JonasToth wrote:
> 
>> I don't know a lot about how to write portable code!
>> 
>> But wouldn't this be out usecase? 
>> http://python-future.org/compatible_idioms.html#file-io-with-open
>> 
>>   Using the `decode` is supposed to work in both pythons
> 
> Yes, indeed, this is more elegant solution!
> 
> https://reviews.llvm.org/D51220


https://reviews.llvm.org/D51220



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LG from my side, but please let @aaron.ballman or @alexfh approve first

Am 24.08.2018 um 18:08 schrieb Andi via Phabricator:

> Abpostelnicu added a comment.
> 
> I can confirm tested on:
>  2.7.15
>  3.7.0
> 
> On both it worked.
> 
> - https://reviews.llvm.org/F7047650: signature.asc 
> https://reviews.llvm.org/F7047650
> 
> https://reviews.llvm.org/D51220


https://reviews.llvm.org/D51220



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

[PATCH] D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340623: [clang-format] fix PR38525 - Extraneous continuation 
indent spaces with… (authored by JonasToth, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D50699

Files:
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTest.cpp


Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -700,7 +700,8 @@
 // Indent relative to the RHS of the expression unless this is a simple
 // assignment without binary expression on the RHS. Also indent relative to
 // unary operators and the colons of constructor initializers.
-State.Stack.back().LastSpace = State.Column;
+if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None)
+  State.Stack.back().LastSpace = State.Column;
   } else if (Previous.is(TT_InheritanceColon)) {
 State.Stack.back().Indent = State.Column;
 State.Stack.back().LastSpace = State.Column;
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3375,6 +3375,18 @@
"= b\n"
"  >> (aa);",
Style);
+
+  Style.ColumnLimit = 80;
+  Style.IndentWidth = 4;
+  Style.TabWidth = 4;
+  Style.UseTab = FormatStyle::UT_Always;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.AlignOperands = false;
+  EXPECT_EQ("return someVeryVeryLongConditionThatBarelyFitsOnALine\n"
+"\t&& (someOtherLongishConditionPart1\n"
+"\t\t|| someOtherEvenLongerNestedConditionPart2);",
+format("return someVeryVeryLongConditionThatBarelyFitsOnALine && 
(someOtherLongishConditionPart1 || someOtherEvenLongerNestedConditionPart2);",
+   Style));
 }
 
 TEST_F(FormatTest, EnforcedOperatorWraps) {


Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -700,7 +700,8 @@
 // Indent relative to the RHS of the expression unless this is a simple
 // assignment without binary expression on the RHS. Also indent relative to
 // unary operators and the colons of constructor initializers.
-State.Stack.back().LastSpace = State.Column;
+if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None)
+  State.Stack.back().LastSpace = State.Column;
   } else if (Previous.is(TT_InheritanceColon)) {
 State.Stack.back().Indent = State.Column;
 State.Stack.back().LastSpace = State.Column;
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3375,6 +3375,18 @@
"= b\n"
"  >> (aa);",
Style);
+
+  Style.ColumnLimit = 80;
+  Style.IndentWidth = 4;
+  Style.TabWidth = 4;
+  Style.UseTab = FormatStyle::UT_Always;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.AlignOperands = false;
+  EXPECT_EQ("return someVeryVeryLongConditionThatBarelyFitsOnALine\n"
+"\t&& (someOtherLongishConditionPart1\n"
+"\t\t|| someOtherEvenLongerNestedConditionPart2);",
+format("return someVeryVeryLongConditionThatBarelyFitsOnALine && (someOtherLongishConditionPart1 || someOtherEvenLongerNestedConditionPart2);",
+   Style));
 }
 
 TEST_F(FormatTest, EnforcedOperatorWraps) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50697: [clang-format] fix PR38557 - comments between "default" and ':' causes the case label to be treated as an identifier

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340624: [clang-format] fix PR38557 - comments between 
"default" and ':' causes the case… (authored by JonasToth, 
committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D50697

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp


Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -350,7 +350,10 @@
   break;
 case tok::kw_default: {
   unsigned StoredPosition = Tokens->getPosition();
-  FormatToken *Next = Tokens->getNextToken();
+  FormatToken *Next;
+  do {
+Next = Tokens->getNextToken();
+  } while (Next && Next->is(tok::comment));
   FormatTok = Tokens->setPosition(StoredPosition);
   if (Next && Next->isNot(tok::colon)) {
 // default not followed by ':' is not a case label; treat it like
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1145,6 +1145,22 @@
"  break;\n"
"}",
Style);
+  Style.ColumnLimit = 80;
+  Style.AllowShortCaseLabelsOnASingleLine = false;
+  Style.IndentCaseLabels = true;
+  EXPECT_EQ("switch (n) {\n"
+"  default /*comments*/:\n"
+"return true;\n"
+"  case 0:\n"
+"return false;\n"
+"}",
+format("switch (n) {\n"
+   "default/*comments*/:\n"
+   "  return true;\n"
+   "case 0:\n"
+   "  return false;\n"
+   "}",
+   Style));
 }
 
 TEST_F(FormatTest, FormatsLabels) {


Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -350,7 +350,10 @@
   break;
 case tok::kw_default: {
   unsigned StoredPosition = Tokens->getPosition();
-  FormatToken *Next = Tokens->getNextToken();
+  FormatToken *Next;
+  do {
+Next = Tokens->getNextToken();
+  } while (Next && Next->is(tok::comment));
   FormatTok = Tokens->setPosition(StoredPosition);
   if (Next && Next->isNot(tok::colon)) {
 // default not followed by ':' is not a case label; treat it like
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1145,6 +1145,22 @@
"  break;\n"
"}",
Style);
+  Style.ColumnLimit = 80;
+  Style.AllowShortCaseLabelsOnASingleLine = false;
+  Style.IndentCaseLabels = true;
+  EXPECT_EQ("switch (n) {\n"
+"  default /*comments*/:\n"
+"return true;\n"
+"  case 0:\n"
+"return false;\n"
+"}",
+format("switch (n) {\n"
+   "default/*comments*/:\n"
+   "  return true;\n"
+   "case 0:\n"
+   "  return false;\n"
+   "}",
+   Style));
 }
 
 TEST_F(FormatTest, FormatsLabels) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162448.
JonasToth added a comment.

- Merge branch 'master' into fix_exception


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

Files:
  clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -35,6 +36,7 @@
 
   try {
 throw non_derived_exception();
+
 // 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) {
@@ -50,24 +52,24 @@
   try {
 throw bad_inheritance();
 // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
-// CHECK MESSAGES: 10:1: note: type defined here
+// CHECK MESSAGES: 11:1: note: type defined here
 throw no_good_inheritance();
 // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
-// CHECK MESSAGES: 11:1: note: type defined here
+// CHECK MESSAGES: 12:1: note: type defined here
 throw really_creative();
 // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
-// CHECK MESSAGES: 12:1: note: type defined here
+// CHECK MESSAGES: 13:1: note: type defined here
   } catch (...) {
   }
   throw bad_inheritance();
   // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
-  // CHECK MESSAGES: 10:1: note: type defined here
+  // CHECK MESSAGES: 11:1: note: type defined here
   throw no_good_inheritance();
   // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
-  // CHECK MESSAGES: 11:1: note: type defined here
+  // CHECK MESSAGES: 12:1: note: type defined here
   throw really_creative();
   // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
-  // CHECK MESSAGES: 12:1: note: type defined here
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +93,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -128,7 +130,7 @@
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   THROW_EXCEPTION(non_derived_exception);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK MESSAGES: 9:1: note: type defined here
+  // CHECK MESSAGES: 10:1: note: type defined here
   THROW_EXCEPTION(std::exception);// Ok
   THROW_EXCEPTION(derived_exception); // Ok
   THROW_EXCEPTION(deep_hierarchy);// Ok
@@ -180,3 +182,21 @@
   // CHECK-NOTES: 169:1: note: type defined here
   throw UsingGood(); // Ok
 }
+
+// Fix PR37913
+struct invalid_argument_maker {
+  ::std::invalid_argument operator()() const;
+};
+struct int_maker {
+  int operator()() const;
+};
+template 
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+void exception_created_with_function() {
+  templated_thrower();
+  templated_thrower();
+
+  throw invalid_argument_maker{}();
+}
Index: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
===
--- clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
+++ clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
@@ -22,24 +22,39 @@
 return;
 
   Finder->addMatcher(
-  cxxThrowExpr(allOf(has(expr(unless(hasType(qualType(hasCanonicalType(
- hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(
- hasName("std::exception")),
- has(expr(unless(cxxUnresolvedConstructExpr(,
- eachOf(has(expr(hasType(namedDecl().bind("decl",
-anything(
+  cxxThrowExpr(
+  allOf(
+  has(expr(unless(hasType(
+  

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162449.
JonasToth added a comment.

- get up to date


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,563 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = &np_local0;
+  int *const p1_np_local0 = &np_local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = &np_local1;
+  int *const p1_np_local1 = &np_local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(&np_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = &np_local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared 'const'
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int &r0_np_local0 = np_local0;
+  int &r1_np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  doubl

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162450.
JonasToth added a comment.

- Merge branch 'master' into check_macros_usage


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -84,6 +84,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 

[PATCH] D40854: [clang-tidy] WIP implement cppcoreguidelines check for mixed integer arithmetic

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162451.
JonasToth added a comment.

- Merge branch 'master' into check_mixed_arithmetic


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40854

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.cpp
  clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-mixed-int-arithmetic.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp

Index: test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp
@@ -0,0 +1,188 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-mixed-int-arithmetic %t
+
+enum UnsignedEnum : unsigned char {
+  UEnum1,
+  UEnum2
+};
+
+enum SignedEnum : signed char {
+  SEnum1,
+  SEnum2
+};
+
+unsigned char returnUnsignedCharacter() { return 42; }
+unsigned returnUnsignedNumber() { return 42u; }
+long returnBigNumber() { return 42; }
+float unrelatedThing() { return 42.f; }
+SignedEnum returnSignedEnum() { return SEnum1; }
+UnsignedEnum returnUnsignedEnum() { return UEnum1; }
+
+void mixed_binary() {
+  unsigned int UInt1 = 42;
+  signed int SInt1 = 42;
+  UnsignedEnum UE1 = UEnum1;
+  SignedEnum SE1 = SEnum1;
+  float UnrelatedFloat = 42.f;
+
+  // Test traditional integer types.
+  auto R1 = UInt1 + SInt1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:21: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+  int R2 = UInt1 - SInt1;
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:20: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:12: note: unsigned operand
+
+  unsigned int R3 = UInt1 * SInt1;
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:29: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand
+
+  unsigned int R4 = UInt1 / returnBigNumber();
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:29: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand
+
+  char R5 = returnUnsignedCharacter() + SInt1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:41: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+  auto R6 = SInt1 - 10u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:13: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand
+
+  auto R7 = UInt1 * 10;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:21: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+  auto R8 = 10u / returnBigNumber();
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:19: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+  auto R9 = 10 + returnUnsignedCharacter();
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:13: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:18: note: unsigned operand
+
+  // Test enum types.
+  char R10 = returnUnsignedEnum() - SInt1;
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:37: note: signed operand
+  // CHECK-MESSAGES: [[@LINE-3]]:14: note: unsigned operand
+
+  unsigned char R11 = returnSignedEnum() * UInt1;
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+  // CHECK-MESSAGES: [[@LINE-2]]:23: note: signed opera