[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

Actually, I already wanted to add an enum, controlling this behavior. 
But even with an enum, this check should only remove includes in consecutive 
blocks, which could be relocated (are not split by `#defines` or a comment)
Or I should add the options to `DeduplicateIncludeDirectives`: 
`{DID_InSameBlock (default), DID_Never, DID_Allways, DID_OnlyConsecutiveBlocks}`

Note, that I use DID_InSameBlock as default, because that's the current 
behavior, even many people would like to have it off by default.

Regarding the breaking of current features: What would you do, if a break is 
necessary, to implement a way more useful feature or to fix a huge issue/bug? 
Would you just infer a new version number? (I personally don't like the idea of 
having version numbers for specific tests, but that would be the easiest way to 
keep compatibility with old configurations and codebases).
Of course, the assumptions must be proven, as you said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143870

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

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D143870#4273075 , @MyDeveloperDay 
wrote:

> I know it might not seem an obvious use case but there really isn't a 
> requirement to not include header files more than once.. imagine if I have
>
>   #define ARCH "win32"
>   #include "MyDataStructThatContainsPlaformSpecificNamesUsingARCH.h"
>   #define ARCH "win64"
>   #include "MyDataStructThatContainsPlaformSpecificNamesUsingARCH.h"
>
> Its not nice, but just because I include it twice doesn't mean its wrong? 
> This change would break code written that way, No?

Mhh, I need to test that. But you are right, if it removes the header in one of 
the conditional preprocessor blocks, it would be very bad.
Regarding normal blocks, the behavior was already inconsistent, as soon to 
reorder blocks was activated:
Blocks are merged if possible, then de-duplicated and split again. So I just 
made the behavior more consistent here.

Regarding the comment that I must not change existing tests: I think this rule 
is too strict, because those tests are mostly regression tests. 
But a regression tests does not test for correctness. So if a test had already 
a wrong assumption, it must be changeable. 
Also, when a behavior is now undesirable, it should be possible to adapt them.
I would say in this case I should replace the test with your case, to reflect 
that includes must not be removed, when they are in different conditional 
preprocessor blocks.
That the different "modi" should be more consistent is in my opinion desirable.

Last but not least, I currently have nearly zero free time, so I would like to 
pause this and my other phab until my thesis is done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143870

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#4224236 , @firewave wrote:

> Some additional thoughts I had a while ago about something I have raised 
> before:
> I think the warnings which can only be fixed with c++14 should either only be 
> issued if that standard was specified or be behind an option. We have lots of 
> lambda captures which could be moved with c++14 and having those warnings 
> would lead to lots of suppressions within the code since we only target c++11.

Yes, I agree, while I can't understand, why someone still wants to use only 
c++11 I can totally understand, that a single Software Engineer can't do 
anything about this, when the corporation does not permit it. 
But it should not be removed, since this warning can still show some pitfalls 
in performance critical code. A way in c++11 to fix the warning, is to create a 
functor, instead of a lambda.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#4222920 , @ccotter wrote:

> @Febbe - checking in. is this still on your radar? I would love to see this 
> merged!

Yes, this is still on my radar, but currently, I am not satisfied with my 
solution. There are too many issues, and I may have to rewrite/fix the whole 
search algorithm.
The last three months I was too busy with both university and my job.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: clang/unittests/Format/SortIncludesTest.cpp:548
+  EXPECT_EQ("#include \"a.h\"\n"
+"#include \"llvm/a.h\"\n"
+"#include \"b.h\"\n"

HazardyKnusperkeks wrote:
> Febbe wrote:
> > HazardyKnusperkeks wrote:
> > > While I may accept there are more than one //main// header, this should 
> > > not happen in my opinion.
> > Yes, that is a conflict of interests. There is no perfect way of 
> > implementing this, without the help of clang-tidy / the clang language 
> > server:
> > 
> > To arguably define a file as a main include, all declarations must be 
> > analyzed, whether they are predeclared in the include or not.
> > But we don't have the help of the clang AST.
> > 
> > My expectation is, that when I randomly shuffle the includes, that they are 
> > always sorted to the same result.
> > Currently, this is not the case, and depending on the rules it can 
> > furthermore happen, that a main include "a.h" is exchanged with the 
> > "llvm/a.h". 
> > This should also not happen, but it does, and it is not covered by the 
> > tests.
> > 
> > I consider the false negative of a correct main include worse than a false 
> > positive.
> > Therefore, I changed it. 
> > In addition, my approach has the advantage that all includes are sorted in 
> > the same way, regardless of the order.
> > 
> > But if you want, we could introduce a new Option like: `enum 
> > FindMainIncludes{FMI_First, FMI_All, FMI_Off = 0};`
> > With `First` to match the current behavior, `All` for the new behavior.
> > But then matched includes with a negative priority would be swapped with 
> > the other possible main_include at each run.
> > I don't know how to prevent this.
> > The best solution I can imagine, is still a comment of the programmer, that 
> > the respective include is or is not a main include.
> > E.g. `// clang-format pragma: no_main_include`
> > 
> > Another idea would be, to insert all main includes into a list with the 
> > matchers' priority, and then take the last negative or the first positive, 
> > but only if it is not partially sortable with the neighbors.
> > This might be a good heuristic, whether the include was previously detected 
> > as main include.
> > But I don't know if this is implementable with the current algorithm.
> > 
> > 
> It is very unfortunate that `llvm/a.h` would be detected as a main header, 
> but would currently the relative order be kept and thus `a.h` always be 
> //the// main header?
> 
> I don't think you will find someone (of the usual reviewers) to be in favor 
> of the pragma, and I certainly don't want such a thing in my code.
> 
> A heuristic seems to be a good way. I'd say from all the candidates we take 
> the one without any directories in it.
> but would currently the relative order be kept and thus a.h always be the 
> main header?

No, unfortunately, clang-format never sorted the path after its dept, 
std::filesystem::path would do that, but clang-format uses the std::less 
operator of StringRef which is a lexicographical comparison.
Therefore, `m.h` will always be ordered after `llvm/m.h`. Changing that would 
reorder nearly all headers in all projects using clang-format. But I could do 
this only for the main-include candidates.

Nevertheless, I don't think this would be a good heuristic either, because me, 
and many others are using an extra `include` folder for public headers (API) 
and the `src` directory for private headers:

```
my-lib/
├─ include/
│  ├─ public_headers.md
│  ├─ a.h
├─ src/
│  ├─ private_headers_and_source.md
│  ├─ a_priv.h
│  ├─ a.cpp
```

With your proposed solution, the primary main header is ordered after the main 
include.
Let me implement my heuristic first, maybe it will work well enough to prevent 
the reordering, when the main header is sorted by hand (just like before, but 
now it also should work for negative priorities).

Otherwise, we must reconsider which tradeoff we choose (maybe a completely new 
one).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: clang/unittests/Format/SortIncludesTest.cpp:548
+  EXPECT_EQ("#include \"a.h\"\n"
+"#include \"llvm/a.h\"\n"
+"#include \"b.h\"\n"

HazardyKnusperkeks wrote:
> While I may accept there are more than one //main// header, this should not 
> happen in my opinion.
Yes, that is a conflict of interests. There is no perfect way of implementing 
this, without the help of clang-tidy / the clang language server:

To arguably define a file as a main include, all declarations must be analyzed, 
whether they are predeclared in the include or not.
But we don't have the help of the clang AST.

My expectation is, that when I randomly shuffle the includes, that they are 
always sorted to the same result.
Currently, this is not the case, and depending on the rules it can furthermore 
happen, that a main include "a.h" is exchanged with the "llvm/a.h". 
This should also not happen, but it does, and it is not covered by the tests.

I consider the false negative of a correct main include worse than a false 
positive.
Therefore, I changed it. 
In addition, my approach has the advantage that all includes are sorted in the 
same way, regardless of the order.

But if you want, we could introduce a new Option like: `enum 
FindMainIncludes{FMI_First, FMI_All, FMI_Off = 0};`
With `First` to match the current behavior, `All` for the new behavior.
But then matched includes with a negative priority would be swapped with the 
other possible main_include at each run.
I don't know how to prevent this.
The best solution I can imagine, is still a comment of the programmer, that the 
respective include is or is not a main include.
E.g. `// clang-format pragma: no_main_include`

Another idea would be, to insert all main includes into a list with the 
matchers' priority, and then take the last negative or the first positive, but 
only if it is not partially sortable with the neighbors.
This might be a good heuristic, whether the include was previously detected as 
main include.
But I don't know if this is implementable with the current algorithm.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe marked 2 inline comments as done.
Febbe added inline comments.



Comment at: clang/lib/Format/Format.cpp:1379
   LLVMStyle.IncludeStyle.IncludeCategories = {
-  {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
-  {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
-  {".*", 1, 0, false}};
+  {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 2, false},
+  {"^(<|\"(gtest|gmock|isl|json)/)", 3, 3, false},

HazardyKnusperkeks wrote:
> Febbe wrote:
> > HazardyKnusperkeks wrote:
> > > I don't follow why this is necessary. And I don't feel comfortable with 
> > > it.
> > This **was** required, but is not required necessarily, my last update of 
> > the patch made this change most likely obsolete.
> > 
> > But the actual reason is, that `SortPriority` is described in the 
> > documentation as "optional value which is defaulted to `Priority`, if 
> > absent", but it is neither a (std::)optional value nor it was defaulted to 
> > Priority if absent.
> > It was indirectly updated from **0** to `Priority` in the algorithm.
> > 
> > Since I moved the buggy, re-initialization of `SortPriority` to the first 
> > initialization:
> > clang/lib/Tooling/Inclusions/IncludeStyle.cpp:L20 this change must also be 
> > covered by the tests: All zero initializations must now be default to 
> > `Priority` to not change the test behavior, which is what you want.
> > 
> > Either by creating a Constructor without `SortPriority`, which defaults to 
> > Priority,
> > creating a factory function, or by doing this for all tests manually.
> > 
> > 
> > Imagine the tests would also test the interpretation of the JSON input. 
> > Then the tests would not require an adjustment to be semantically equal.
> > 
> > But since I added Priority to the sorting after **this** change, this is 
> > kind of irrelevant.
> My main question is, does this change behavior? In either case, please add 
> some tests to show the difference, or the lack thereof.
I removed all changes to the tests, which do not change the behavior.

One test required a change. It assumed, that only one file can be a main 
include, which is, in my opinion, not correct. I've made the change, that all 
files, which are matched as main include are ordered with priority 0 (this 
especially means includes introduced by `IncludeIsMainRegex` and 
`IncludeIsMainSourceRegex`). The previous implementation ignored those, if 
another main file was found earlier. It could also result in unpredictable 
resorting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 497701.
Febbe added a comment.

Added requested tests:

- since we now support priorities, equal to 0 (the main include priority) 
sorting should work now for those.
- multiple files can be main-includes now, the tests expects that now.
- negative priorities do not override the main include priority anymore, which 
is also tested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/lib/Tooling/Inclusions/IncludeStyle.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -8,6 +8,7 @@
 
 #include "FormatTestUtils.h"
 #include "clang/Format/Format.h"
+#include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Debug.h"
 #include "gtest/gtest.h"
@@ -535,16 +536,48 @@
  "#include \"b.h\"\n",
  "a.cc"));
 
-  // Only recognize the first #include with a matching basename as main include.
+  // Todo (remove-before-merge): I consider the assumption, that there is only
+  // one main include as wrong.
+  // E.g. a.cpp -> a.priv.h && a.h
+  // E.g. a_test.cpp -> a_test.h && a.h
+  // Maybe add a "//clang-format pragma: not_main" to remove false positives
+
+  // Recognize all possible main #include's with a matching basename as main
+  // include.
+  EXPECT_EQ("#include \"a.h\"\n"
+"#include \"llvm/a.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n",
+sort("#include \"b.h\"\n"
+ "#include \"a.h\"\n"
+ "#include \"c.h\"\n"
+ "#include \"llvm/a.h\"\n",
+ "a.cc"));
+
+  // Keep manually splitted groups in place
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"b.h\"\n"
 "#include \"c.h\"\n"
+"// no main:\n"
 "#include \"llvm/a.h\"\n",
 sort("#include \"b.h\"\n"
  "#include \"a.h\"\n"
  "#include \"c.h\"\n"
+ "// no main:\n"
  "#include \"llvm/a.h\"\n",
  "a.cc"));
+
+  // Keep both main files together (with priority 0)
+  Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  EXPECT_EQ("#include \"a.h\"\n"
+"#include \"a_test.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n",
+sort("#include \"b.h\"\n"
+ "#include \"a.h\"\n"
+ "#include \"c.h\"\n"
+ "#include \"a_test.h\"\n",
+ "a_test.cc"));
 }
 
 TEST_F(SortIncludesTest, LeavesMainHeaderFirstInAdditionalExtensions) {
@@ -762,6 +795,64 @@
  "#include \"c_main.h\"\n"
  "#include \"a_other.h\"\n",
  "c_main.cc", 0));
+  // All negative
+  Style.IncludeCategories = {{".*important_os_header.*", -3, 0, false},
+ {".*", -2, 0, false}};
+
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+"#include \"a_other.h\"\n"
+"#include \"c_main.h\"\n",
+sort("#include \"c_main.h\"\n"
+ "#include \"a_other.h\"\n"
+ "#include \"important_os_header.h\"\n",
+ "c_main.cc"));
+}
+
+TEST_F(SortIncludesTest, ZeroPriorities) {
+  Style.IncludeCategories = {{".*important_os_header.*", 0, -1, false},
+ {".*", 1, 0, false}};
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+"#include \"c_main.h\"\n"
+"#include \"a_other.h\"\n",
+sort("#include \"c_main.h\"\n"
+ "#include \"a_other.h\"\n"
+ "#include \"important_os_header.h\"\n",
+ "c_main.cc"));
+
+  // check stable when re-run
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+"#include \"c_main.h\"\n"
+"#include \"a_other.h\"\n",
+sort("#include \"important_os_header.h\"\n"
+ "#include \"c_main.h\"\n"
+ "#include \"a_other.h\"\n",
+ "c_main.cc", 0));
+
+  // All zero, sorted after name
+  Style.IncludeCategories = {{".*important_os_header.*", 0, 0, false},
+ {".*", 0, 0, false}};
+
+  EXPECT_EQ("#include \"a_other.h\"\n"
+"#include \"c_main.h\"\n"
+"#include \"important_os_header.h\"\n",
+sort("#include \"important_os_header.h\"\n"
+ "#include \"c_main.h\"\n"
+ "#include \"a_other.h\"\n",
+ "c_main.cc"));
+
+  // Grouped:
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = 

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 497687.
Febbe marked 3 inline comments as done.
Febbe added a comment.

- Removed some, not required changes.
- Reverted mapOptional -> mapRequired change, since it might break existing 
configs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/lib/Tooling/Inclusions/IncludeStyle.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -535,11 +535,18 @@
  "#include \"b.h\"\n",
  "a.cc"));
 
-  // Only recognize the first #include with a matching basename as main include.
+  // Todo (remove-before-merge): I consider the assumption, that there is only
+  // one main include as wrong.
+  // E.g. a.cpp -> a.priv.h && a.h
+  // E.g. a_test.cpp -> a_test.h && a.h
+  // Maybe add a "//clang-format pragma: not_main" to remove false positives
+
+  // Recognize all possible main #include's with a matching basename as main
+  // include.
   EXPECT_EQ("#include \"a.h\"\n"
+"#include \"llvm/a.h\"\n"
 "#include \"b.h\"\n"
-"#include \"c.h\"\n"
-"#include \"llvm/a.h\"\n",
+"#include \"c.h\"\n",
 sort("#include \"b.h\"\n"
  "#include \"a.h\"\n"
  "#include \"c.h\"\n"
Index: clang/lib/Tooling/Inclusions/IncludeStyle.cpp
===
--- clang/lib/Tooling/Inclusions/IncludeStyle.cpp
+++ clang/lib/Tooling/Inclusions/IncludeStyle.cpp
@@ -7,17 +7,26 @@
 //===--===//
 
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
+#include 
 
 using clang::tooling::IncludeStyle;
 
 namespace llvm {
 namespace yaml {
 
+// Todo (remove before merge): changes here are required,
+// because the explicit override for the default values of 0 are moved from
+// the algorithm to this place
+// Since we can't make those values required, we must set the previously
+// intended default values here, to prevent behaviour changes.
+
+constexpr int DefaultPriority = std::numeric_limits::max();
+
 void MappingTraits::mapping(
 IO , IncludeStyle::IncludeCategory ) {
   IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapOptional("Priority", Category.Priority, DefaultPriority);
+  IO.mapOptional("SortPriority", Category.SortPriority, Category.Priority);
   IO.mapOptional("CaseSensitive", Category.RegexIsCaseSensitive);
 }
 
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -12,6 +12,7 @@
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
+#include 
 #include 
 
 namespace clang {
@@ -206,33 +207,33 @@
   }
 }
 
+constexpr int DefaultMainIncludePriority = 0;
+constexpr int DefaultMainIncludeSortPriority = 0;
+
 int IncludeCategoryManager::getIncludePriority(StringRef IncludeName,
bool CheckMainHeader) const {
-  int Ret = INT_MAX;
+  if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+return DefaultMainIncludePriority;
+
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
-if (CategoryRegexs[i].match(IncludeName)) {
-  Ret = Style.IncludeCategories[i].Priority;
-  break;
-}
-  if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
-Ret = 0;
-  return Ret;
+if (CategoryRegexs[i].match(IncludeName))
+  return Style.IncludeCategories[i].Priority;
+
+  return std::numeric_limits::max();
 }
 
 int IncludeCategoryManager::getSortIncludePriority(StringRef IncludeName,
bool CheckMainHeader) const {
-  int Ret = INT_MAX;
+  if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+return DefaultMainIncludeSortPriority;
+
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
-if (CategoryRegexs[i].match(IncludeName)) {
-  Ret = Style.IncludeCategories[i].SortPriority;
-  if (Ret == 0)
-Ret = Style.IncludeCategories[i].Priority;
-  break;
-}
-  if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
-Ret = 0;
-  return Ret;
+if (CategoryRegexs[i].match(IncludeName))
+  return Style.IncludeCategories[i].SortPriority;
+
+  return std::numeric_limits::max();
 }
+
 bool 

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

@HazardyKnusperkeks thank you for the review, I would add another option, but I 
don't know a good name. I would propose a

`boolean` `IncludeDeduplicateInAllBlocks` which defaults to zero.

First an `Include`, to keep include-sorting related options together in the 
documentation (sorted by name not by category)
`DeduplicateInAllBlocks` self explaining name.

But `IncludeDeduplication` with the enum states `IDD_SameBlock`(default) and 
IDD_ALL would also an option.
The latter has the advantage, that an expansion of this option would not 
require the adjustment of tests.

What's your preference?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143870

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

I've added some elaborations and justifications for the criticized changes.




Comment at: clang/lib/Format/Format.cpp:1379
   LLVMStyle.IncludeStyle.IncludeCategories = {
-  {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
-  {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
-  {".*", 1, 0, false}};
+  {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 2, false},
+  {"^(<|\"(gtest|gmock|isl|json)/)", 3, 3, false},

HazardyKnusperkeks wrote:
> I don't follow why this is necessary. And I don't feel comfortable with it.
This **was** required, but is not required necessarily, my last update of the 
patch made this change most likely obsolete.

But the actual reason is, that `SortPriority` is described in the documentation 
as "optional value which is defaulted to `Priority`, if absent", but it is 
neither a (std::)optional value nor it was defaulted to Priority if absent.
It was indirectly updated from **0** to `Priority` in the algorithm.

Since I moved the buggy, re-initialization of `SortPriority` to the first 
initialization:
clang/lib/Tooling/Inclusions/IncludeStyle.cpp:L20 this change must also be 
covered by the tests: All zero initializations must now be default to 
`Priority` to not change the test behavior, which is what you want.

Either by creating a Constructor without `SortPriority`, which defaults to 
Priority,
creating a factory function, or by doing this for all tests manually.


Imagine the tests would also test the interpretation of the JSON input. Then 
the tests would not require an adjustment to be semantically equal.

But since I added Priority to the sorting after **this** change, this is kind 
of irrelevant.



Comment at: clang/lib/Tooling/Inclusions/IncludeStyle.cpp:18
 IO , IncludeStyle::IncludeCategory ) {
-  IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapRequired("Regex", Category.Regex);
+  IO.mapRequired("Priority", Category.Priority);

HazardyKnusperkeks wrote:
> A big no! You will break existing configurations.
Can you elaborate?
Do you mean the change from `mapOptional` to `mapRequired`?
From what I thought, this change only affects clarity (improvement), not the 
semantic:
 - the parent JSON list/array entry is already optional.
 - therefore any of them is required, but none is useful alone.
   - an empty regex, just matches nothing `=>` regex required
   - an empty Priority should default (documentation) to INT_MAX, but actually 
did not (0). 
 - Also, from the documentations view, omitting one of them was never intended.

The only effect this change will have for old configurations, is, that it 
breaks "wrong" configurations. It's like abusing undefined behav.: You can't be 
sure it works after a compiler update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

[Fabian Keßler via Phabricator via cfe-commits]

Febbe created this revision.
Herald added a project: All.
Febbe requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This change aims to remove all duplicate include directives, 
instead of only those, which are in the same block.

This change might be a bit controversial, since it will also remove includes, 
which are in custom splitted blocks like:

  #include "a.h"
  /* some code */
  
  // following include must stay!:
  #include "a.h"

This is a follow up patch for D143691 , but 
can be freestanding.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143870

Files:
  clang/lib/Format/Format.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -268,9 +268,7 @@
 "#include \n"
 "#include \n"
 "/* clang-format offically */\n"
-"#include \n"
-"#include \n"
-"#include \n"
+"\n"
 "/* clang-format onwards */\n",
 sort("#include \n"
  "#include \n"
@@ -899,6 +897,19 @@
  "\n"
  "#include \n"
  "#include \n"));
+
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
+  EXPECT_EQ("#include \"MyProject/MyLibrary/c_main.h\"\n"
+"\n"
+"#include \n"
+"//\n",
+sort("#include \"MyProject/MyLibrary/c_main.h\"\n"
+ "#include \"MyProject/MyLibrary/c_main.h\"\n"
+ "#include \n"
+ "#include \n"
+ "//\n"
+ "#include \n",
+ "c_main.cpp", 2U));
 }
 
 TEST_F(SortIncludesTest, CalculatesCorrectCursorPositionAfterDeduplicate) {
@@ -924,11 +935,10 @@
   EXPECT_EQ(26u, newCursor(Code, 52));
 }
 
-TEST_F(SortIncludesTest, DeduplicateLocallyInEachBlock) {
+TEST_F(SortIncludesTest, DeduplicateInAllBlocks) {
   EXPECT_EQ("#include \n"
 "#include \n"
 "\n"
-"#include \n"
 "#include \n",
 sort("#include \n"
  "#include \n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -38,6 +38,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
@@ -2858,6 +2859,7 @@
 // #include in the duplicate #includes.
 static void sortCppIncludes(const FormatStyle ,
 const SmallVectorImpl ,
+llvm::StringSet<> ,
 ArrayRef Ranges, StringRef FileName,
 StringRef Code, tooling::Replacements ,
 unsigned *Cursor) {
@@ -2900,12 +2902,14 @@
   }
 
   // Deduplicate #includes.
-  Indices.erase(std::unique(Indices.begin(), Indices.end(),
-[&](unsigned LHSI, unsigned RHSI) {
-  return Includes[LHSI].Text.trim() ==
- Includes[RHSI].Text.trim();
-}),
-Indices.end());
+
+  Indices.erase(
+  std::remove_if(
+  Indices.begin(), Indices.end(),
+  [&](unsigned val) {
+return !AllIncludes.try_emplace(Includes[val].Text.trim()).second;
+  }),
+  Indices.end());
 
   int CurrentCategory = Includes.front().Category;
 
@@ -2966,6 +2970,7 @@
   .Default(0);
   unsigned SearchFrom = 0;
   SmallVector Matches;
+  llvm::StringSet<> AllIncludes{};
   SmallVector IncludesInBlock;
 
   // FIXME: Do some validation, e.g. edit distance of the base name, to fix
@@ -3032,16 +3037,20 @@
 Categories.getSortIncludePriority(IncludeName, FirstIncludeBlock);
 // Todo(remove before merge): reason for removal: every header can have
 // 0,0 priority
+
 IncludesInBlock.push_back(
 {IncludeName, Line, Prev, Category, Priority});
+
   } else if (!IncludesInBlock.empty() && !EmptyLineSkipped) {
-sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code,
-Replaces, Cursor);
+sortCppIncludes(Style, IncludesInBlock, AllIncludes, Ranges, FileName,
+Code, Replaces, Cursor);
 IncludesInBlock.clear();
-if (Trimmed.startswith("#pragma hdrstop")) // Precompiled headers.
+if (Trimmed.startswith("#pragma hdrstop")) { // Precompiled headers.
   FirstIncludeBlock = true;
-else
+  AllIncludes.clear();
+} 

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

I think I am done, and you can review it now.

With the latest changes, SortPriority is no longer required to match Priority, 
it can always be 0, since Priority is now used in the sorting algorithm.
This is what the documentation tends to depict:

"and #includes are sorted first according to increasing category number and 
then alphabetically within each category."
" There is a third and optional field SortPriority which can used while 
IncludeBlocks = IBS_Regroup to define the priority in which #includes should be 
ordered."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 496650.
Febbe added a comment.

Added priority to the sorting algorithm.
This prevents unwanted splits and weird resorts of groups with the same 
priority, but with different and overlapping (other groups) SortPriority.
The new system can now be understood as Primary.SecondaryPriority instead of:

sort by SecondPriority + bucket By PrimaryPriority


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/lib/Tooling/Inclusions/IncludeStyle.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -87,7 +87,7 @@
 TEST_F(SortIncludesTest, SortedIncludesUsingSortPriorityAttribute) {
   FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   FmtStyle.IncludeStyle.IncludeCategories = {
-  {"^", 1, 0, false},
+  {"^", 1, 1, false},
   {"^", 1, 1, false},
   {"^ a.priv.h && a.h
+  // E.g. a_test.cpp -> a_test.h && a.h
+  // Maybe add a "//clang-format pragma: not_main" to remove false positives
+
+  // Recognize all possible main #include's with a matching basename as main
+  // include.
   EXPECT_EQ("#include \"a.h\"\n"
+"#include \"llvm/a.h\"\n"
 "#include \"b.h\"\n"
-"#include \"c.h\"\n"
-"#include \"llvm/a.h\"\n",
+"#include \"c.h\"\n",
 sort("#include \"b.h\"\n"
  "#include \"a.h\"\n"
  "#include \"c.h\"\n"
@@ -645,8 +652,9 @@
  "a.h"));
 
   Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
-  Style.IncludeCategories = {
-  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+  Style.IncludeCategories = {{"^\"", 1, 1, false}, //
+ {"^<.*\\.h>$", 2, 2, false},
+ {"^<", 3, 3, false}};
 
   StringRef UnsortedCode = "#include \"qt.h\"\n"
"#include \n"
@@ -695,11 +703,11 @@
 
 TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveMachting) {
   Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
-  Style.IncludeCategories = {{"^\"", 1, 0, false},
- {"^<.*\\.h>$", 2, 0, false},
- {"^", 3, 0, false},
- {"^", 4, 0, false},
- {"^<", 5, 0, false}};
+  Style.IncludeCategories = {{"^\"", 1, 1, false},
+ {"^<.*\\.h>$", 2, 2, false},
+ {"^", 3, 3, false},
+ {"^", 4, 4, false},
+ {"^<", 5, 5, false}};
 
   StringRef UnsortedCode = "#include \n"
"#include \"qt.h\"\n"
@@ -744,8 +752,8 @@
 }
 
 TEST_F(SortIncludesTest, NegativePriorities) {
-  Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
- {".*", 1, 0, false}};
+  Style.IncludeCategories = {{".*important_os_header.*", -1, -1, false},
+ {".*", 1, 1, false}};
   EXPECT_EQ("#include \"important_os_header.h\"\n"
 "#include \"c_main.h\"\n"
 "#include \"a_other.h\"\n",
@@ -765,8 +773,8 @@
 }
 
 TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {
-  Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
- {".*", 1, 0, false}};
+  Style.IncludeCategories = {{".*important_os_header.*", -1, -1, false},
+ {".*", 1, 1, false}};
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
 
   EXPECT_EQ("#include \"important_os_header.h\"\n"
Index: clang/lib/Tooling/Inclusions/IncludeStyle.cpp
===
--- clang/lib/Tooling/Inclusions/IncludeStyle.cpp
+++ clang/lib/Tooling/Inclusions/IncludeStyle.cpp
@@ -15,9 +15,9 @@
 
 void MappingTraits::mapping(
 IO , IncludeStyle::IncludeCategory ) {
-  IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapRequired("Regex", Category.Regex);
+  IO.mapRequired("Priority", Category.Priority);
+  IO.mapOptional("SortPriority", Category.SortPriority, Category.Priority);
   IO.mapOptional("CaseSensitive", Category.RegexIsCaseSensitive);
 }
 
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -12,6 +12,7 @@
 #include "clang/Lex/Lexer.h"
 #include 

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 496647.
Febbe added a comment.

Fixed the Unit Tests

- rewritten one test, which made the assumption, that there can be only one 
main header.
  - it now asserts, that all matching headers are considered as main header.
- replaced initialisations of `IncludeCategories.SortPriority` to zero  with 
the value from `IncludeCategories.Priority`
  - the previous approach to set the SortPriority when it's 0 to Priority, 
instead of initializing it directly as intended had several drawbacks:
- values of 0 were not possible and resulted in weird behav.
- when a main include was matched by another matcher, it got annother 
sortpriority than 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/lib/Tooling/Inclusions/IncludeStyle.cpp
  clang/unittests/Format/SortIncludesTest.cpp

Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -87,7 +87,7 @@
 TEST_F(SortIncludesTest, SortedIncludesUsingSortPriorityAttribute) {
   FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   FmtStyle.IncludeStyle.IncludeCategories = {
-  {"^", 1, 0, false},
+  {"^", 1, 1, false},
   {"^", 1, 1, false},
   {"^ a.priv.h && a.h
+  // E.g. a_test.cpp -> a_test.h && a.h
+  // Maybe add a "//clang-format pragma: not_main" to remove false positives
+
+  // Recognize all possible main #include's with a matching basename as main
+  // include.
   EXPECT_EQ("#include \"a.h\"\n"
+"#include \"llvm/a.h\"\n"
 "#include \"b.h\"\n"
-"#include \"c.h\"\n"
-"#include \"llvm/a.h\"\n",
+"#include \"c.h\"\n",
 sort("#include \"b.h\"\n"
  "#include \"a.h\"\n"
  "#include \"c.h\"\n"
@@ -645,8 +652,9 @@
  "a.h"));
 
   Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
-  Style.IncludeCategories = {
-  {"^\"", 1, 0, false}, {"^<.*\\.h>$", 2, 0, false}, {"^<", 3, 0, false}};
+  Style.IncludeCategories = {{"^\"", 1, 1, false}, //
+ {"^<.*\\.h>$", 2, 2, false},
+ {"^<", 3, 3, false}};
 
   StringRef UnsortedCode = "#include \"qt.h\"\n"
"#include \n"
@@ -695,11 +703,11 @@
 
 TEST_F(SortIncludesTest, SupportOptionalCaseSensitiveMachting) {
   Style.IncludeBlocks = clang::tooling::IncludeStyle::IBS_Regroup;
-  Style.IncludeCategories = {{"^\"", 1, 0, false},
- {"^<.*\\.h>$", 2, 0, false},
- {"^", 3, 0, false},
- {"^", 4, 0, false},
- {"^<", 5, 0, false}};
+  Style.IncludeCategories = {{"^\"", 1, 1, false},
+ {"^<.*\\.h>$", 2, 2, false},
+ {"^", 3, 3, false},
+ {"^", 4, 4, false},
+ {"^<", 5, 5, false}};
 
   StringRef UnsortedCode = "#include \n"
"#include \"qt.h\"\n"
@@ -744,8 +752,8 @@
 }
 
 TEST_F(SortIncludesTest, NegativePriorities) {
-  Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
- {".*", 1, 0, false}};
+  Style.IncludeCategories = {{".*important_os_header.*", -1, -1, false},
+ {".*", 1, 1, false}};
   EXPECT_EQ("#include \"important_os_header.h\"\n"
 "#include \"c_main.h\"\n"
 "#include \"a_other.h\"\n",
@@ -765,8 +773,8 @@
 }
 
 TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {
-  Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
- {".*", 1, 0, false}};
+  Style.IncludeCategories = {{".*important_os_header.*", -1, -1, false},
+ {".*", 1, 1, false}};
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
 
   EXPECT_EQ("#include \"important_os_header.h\"\n"
Index: clang/lib/Tooling/Inclusions/IncludeStyle.cpp
===
--- clang/lib/Tooling/Inclusions/IncludeStyle.cpp
+++ clang/lib/Tooling/Inclusions/IncludeStyle.cpp
@@ -15,9 +15,9 @@
 
 void MappingTraits::mapping(
 IO , IncludeStyle::IncludeCategory ) {
-  IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapRequired("Regex", Category.Regex);
+  IO.mapRequired("Priority", Category.Priority);
+  IO.mapOptional("SortPriority", Category.SortPriority, Category.Priority);
   IO.mapOptional("CaseSensitive", Category.RegexIsCaseSensitive);
 }
 
Index: 

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:16
 #include 
 
 namespace clang {

`` is used, but not included. Currently, it works, since another header 
already included it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

Still some issues with: SortPriorities
The following settings (note the SortPriority of '^<.*' == 1) which will 
produce an extra group for the attached includes:

  IncludeCategories:
- Regex:   '^'
  Priority:2
  SortPriority:2
- Regex:   '^<.*\.h>'
  Priority:1
  SortPriority:1
- Regex:   '^<.*'
  Priority:2
  SortPriority:1
- Regex:   '.*'
  Priority:-1
  SortPriority:1



  #include "toolset_febbe.h"
  
  #include "zhello_world.h"
  
  #include 
  #include 
  
  #include 
  
  #include  // <- no group expected

But at least it does what the documentation states and the most users would 
expect for the default case (Priority == SortPriority).

It seems that the include list is currently first sorted (by SortPriority), and 
then split into groups, which does not make much sense, since it also 
rearranges the groups and splits them up, but the sorting before the regroup is 
at least documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691

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

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe created this revision.
Herald added a project: All.
Febbe requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Main Include Files have now always prio (0,0)
- They can't be matched by negative matchers anymore.
- `SortPriority` now truly defaults to `Priority`
- If it is unclear, which include is the main include, use both instead of 
using the first.

fixes #58284
fixes #60589


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143691

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/lib/Tooling/Inclusions/IncludeStyle.cpp

Index: clang/lib/Tooling/Inclusions/IncludeStyle.cpp
===
--- clang/lib/Tooling/Inclusions/IncludeStyle.cpp
+++ clang/lib/Tooling/Inclusions/IncludeStyle.cpp
@@ -15,9 +15,9 @@
 
 void MappingTraits::mapping(
 IO , IncludeStyle::IncludeCategory ) {
-  IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapRequired("Regex", Category.Regex);
+  IO.mapRequired("Priority", Category.Priority);
+  IO.mapOptional("SortPriority", Category.SortPriority, Category.Priority);
   IO.mapOptional("CaseSensitive", Category.RegexIsCaseSensitive);
 }
 
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -206,33 +206,35 @@
   }
 }
 
+constexpr int DefaultMainIncludePriority = 0;
+constexpr int DefaultMainIncludeSortPriority = 0;
+
 int IncludeCategoryManager::getIncludePriority(StringRef IncludeName,
bool CheckMainHeader) const {
-  int Ret = INT_MAX;
+  if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+return DefaultMainIncludePriority;
+
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
 if (CategoryRegexs[i].match(IncludeName)) {
-  Ret = Style.IncludeCategories[i].Priority;
-  break;
+  return Style.IncludeCategories[i].Priority;
 }
-  if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
-Ret = 0;
-  return Ret;
+
+  return std::numeric_limits::max();
 }
 
 int IncludeCategoryManager::getSortIncludePriority(StringRef IncludeName,
bool CheckMainHeader) const {
-  int Ret = INT_MAX;
+  if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+return DefaultMainIncludeSortPriority;
+
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
 if (CategoryRegexs[i].match(IncludeName)) {
-  Ret = Style.IncludeCategories[i].SortPriority;
-  if (Ret == 0)
-Ret = Style.IncludeCategories[i].Priority;
-  break;
+  return Style.IncludeCategories[i].SortPriority;
 }
-  if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
-Ret = 0;
-  return Ret;
+
+  return std::numeric_limits::max();
 }
+
 bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const {
   if (!IncludeName.startswith("\""))
 return false;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2966,16 +2966,12 @@
   SmallVector Matches;
   SmallVector IncludesInBlock;
 
-  // In compiled files, consider the first #include to be the main #include of
-  // the file if it is not a system #include. This ensures that the header
-  // doesn't have hidden dependencies
-  // (http://llvm.org/docs/CodingStandards.html#include-style).
-  //
   // FIXME: Do some validation, e.g. edit distance of the base name, to fix
   // cases where the first #include is unlikely to be the main header.
   tooling::IncludeCategoryManager Categories(Style.IncludeStyle, FileName);
   bool FirstIncludeBlock = true;
-  bool MainIncludeFound = false;
+  // Todo(remove before merge): removed: multiple files can be main includes
+  // This option caused random sorting, depending which was detected first.
   bool FormattingOff = false;
 
   // '[' must be the first and '-' the last character inside [...].
@@ -3029,11 +3025,11 @@
 }
 int Category = Categories.getIncludePriority(
 IncludeName,
-/*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
-int Priority = Categories.getSortIncludePriority(
-IncludeName, !MainIncludeFound && FirstIncludeBlock);
-if (Category == 0)
-  MainIncludeFound = true;
+/*CheckMainHeader=*/FirstIncludeBlock);
+int Priority =
+Categories.getSortIncludePriority(IncludeName, FirstIncludeBlock);
+// Todo(remove before merge): reason for removal: every header can have
+// 0,0 priority
 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#4047828 , @Skylion007 
wrote:

> I am trying this in the wild and getting some false positives where it tries 
> to call std::move inside loop conditions and in the boolean condition for an 
> if statement. Stuff like:
>
>   if (SmartPtr new_ptr = steal_ptr(std::move(old_ptr))) {
> ...
>   }
>   else {
> return old_ptr; // Now it's moved from and invalid
>   }
>
> Also this can be happen for similar boolean conditionals in while, for, do 
> while loops.
>
> Interestingly, the logic in bugprone-use-after-move flags this error so maybe 
> we can reuse some of that logic to detect bad std::move.

Does this only happen on "init-statements" of while/if/switch?
And thanks for the catch, I'll take a look into it as soon I have more time.

Btw. I've oriented myself at the "bugprone-use-after-move" check, but simply 
inverting it does not work here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp:18
   // CHECK-NOTES-C:(ptrdiff_t)( )
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider 
type

v1nh1shungry wrote:
> Actually I have tried adding
> 
> ```
> // CHECK-FIXES-C: return [(ptrdiff_t)(a * b)];
> // CHECK-FIXES-CXX: return [static_cast(a * b)];
> ```
> 
> under this line, but the test failed, and when I took a look at 
> `build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/implicit-widening-of-multiplication-result-array-subscript-expression.cpp.tmp.cpp`,
>  I found that these codes didn't get modified. And I took a look at other 
> files which have `CHECK-FIXES` lines, I found the codes in the corresponding 
> temporary files got fixed.
Maybe, because the Fixup is marked as Note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D141058#4028768 , @v1nh1shungry 
wrote:

> Sorry, I don't know how to add tests for such fixes. Could anyone please give 
> me some hints? Thanks!

It seems like the tests for this check do not contain checks for the fixups 
itself.

Normally, they are tested with `CHECK-FIXES: ` for example:

  return F? Mov: Movable{}; 
  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last 
use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
  // CHECK-FIXES: return F? std::move(Mov): Movable{};

In the case of this test file, you have to append the suffixes to the 
`CHECK-FIXES` to `CHECK-FIXES-` where `` is one of `C`, `CXX`, 
`ALL`

  return F? Mov: Movable{}; 
  // CHECK-MESSAGES-CXX: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on 
last use, consider moving it instead. 
[performance-unnecessary-copy-on-last-use] 
  // CHECK-FIXES-CXX: return F? std::move(Mov): Movable{};




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:215
   (Twine("static_cast<") + TyAsString + ">(").str())
-   << FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
 else

Actually, I find it pretty weird/suspicious, that ` -> 
getEndLoc()` does not return the end of its `Expr`. The code looked correct 
already.
Can you elaborate, if this is a bug in the `getEndLoc()` of those `Expr`s? It 
might be better to fix it there directly.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#4018548 , @ccotter wrote:

> @Febbe - Really glad to see this phab up! Not having realized this phab was 
> in progress, I implemented this a few months back (though, originally not as 
> a clang-tidy tool and never published) and thought it'd be worth sharing 
> notes. I recently turned my non-clang-tidy implementation into a clang-tidy 
> one here 
> .
>  A couple suggestions based on my experience writing a similar tool,

Hello @ccotter cool, you found this phab, and that you also had the same idea. 
I would like it, to collaborate with you. I just don't have a clue how to do 
this properly and clean via phabricator. What's your suggestion?

> - Should we detect "escaped" values, i.e., values who have their address 
> taken, and not apply a FixIt in these cases? See this test 
> .

Currently, this check does not care, if a value is taken by reference or 
pointer, not even when the value is taken in the same scope.
This is in fact an open Todo.
A rule of thumb would be: if the called function is analyzable, analyze it. 
Else, declare the move safety as unsecure and do warn, if the user is 
interested in it.

Generally I think, that fix it's should be always displayed, when they produce 
compilable code. But the move safety should be displayed in the error message.
Of course, it would be good to disable questionable fixups for automatic fix up 
runs. But I didn't heart of a solution to differ in a clang-tidy check, between 
automatic fixups and those used in an IDE.

> - I took a slightly more conservative approach in only moving values whose 
> `QualType.isTrivialType()` is true, which addresses the 
> `shared_ptr` problem. We can then apply FixIts for containers 
> (std::vector etc) and algebraic types (tuple/optional) of said types (and 
> containers of containers of ...). In general, the approach I was aiming for 
> was to define a classification of "safe to move types" (inferred from 
> isTrivialType, and/or using a list of safe types of in the tool configuration 
> as you have done). Containers/algebraic types of safe to move types, or smart 
> pointers of safe to move types (being careful with any custom allocator which 
> might lead to a similar problem as the scope guard case) are also safe to 
> move, etc. Having the tool be able to distinguish "safe to move" FixIts from 
> "potentially unsafe" (as a tool mode, potentially with "safe" as the default) 
> would ensure safe refactoring.

But moving trivial types are equal to a copy, aren't they? Therefore, I search 
for types where this is false. 
I also think, because clang-tidy does not claim to be 100% correct, in contrast 
to a compiler which must be, having such cases like `shared_ptr` is 
acceptable. Mostly such cases also have a huge smell (store mutex along with 
the data etc...).

> - This might be a nitpick, but in the diagnostic message `warning: Parameter 
> '...'`, "Parameter" typically means something more specific (the names given 
> to function variables, aka, function parameters). How about `warning: Value 
> '...'`

Thank you for the catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

>> It is also completely irrelevant, because a new programmer will not 
>> understand that const T const * is actually T const* and not T const * 
>> const. An experienced programmer can understand it well either way.
>
> not sure I follow the argument here, but surely I also agree that east consts 
> are more readable and fool-proof. it's unfortunate that most of the C++ world 
> is stuck with the west consts.

Exactly this.

> surely, it would be nice to have a clang-tidy check or something (by design 
> clang-format only performs white-space changes).

It would also be good to have some sort of refactoring choice in clangd to swap 
the alignment. 
Maybe we should add a feature request on GitHub for both.

> FWIW, one reason this isn't enabled is that it's relatively recent and also 
> not completely safe in principle.

It's not as simple as "if LLVM cares about const-alignment, this flag should be 
on".

I also was curious about that, until I've read the documentation about this 
formatter option. But is it safe for already correct alignments?
Then it could be used in the linting step to just verify, that it is done 
correctly, am I right?

Back to topic
-

I have some open questions:

@njames93 asked me to remove the explicitly deleted copy/move 
constructors/assignment operators.
But this would break the rule of 5, or I have to move the `CFG.h` include into 
the header which is in my opinion also against the coding convention.
Is it ok to keep them, should I break the rule of 5 or should I move the 
`CFG.h` include into the header?

@njames93 also mentioned, that the complete traversal of the Context is a waste 
of resources. 
But I have no clue, how to use the Context down from the top function.
Is there even a way to do so, or do I have to start with `TraverseFunctionDecl` 
in the `RecursiveASTVisitor`?

An even better approach would be to start with the DeclRefExpr itself. 
Is there a way to traverse a matcher in the reverse order from the DeclRefExpr 
as start?
Or do I have to parse it by hand then?

Currently, I got many reviews regarding the coding stile and the result, but I 
would also like to see a review to the CFG parsing itself.

There are also "open improvements", but they require a huge amount of work on 
top of this patch:

- fixups in lambda captures:
  - To fixup lambda captures sustainably, like the handling of multiple last 
usages in the capture list / implicit capture. The new capture list must be 
built at the end of the parsing of the translation unit.
  - Some sort of capture-list builder must be used, which first collects all 
last usages, and then creates one single fix up for all.
  - In my opinion, this is non-blocking since the benefit is small compared to 
the work.
  - I am willing to add this, if desired, because I have a plan how I would 
implement it.
- A heuristic to catch copyable but non trivially copyable and movable "guards".
  - This is a lot harder, and I actually have no clue how to do this without 
scanning each type __recursively__ for side effects to child fields in any 
destructor which are not directly followed by memory management (destruction).
  - I would like to defer this.
- There is no detection of last usages of fields
  - This is also non-blocking, since it is a pure feature.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 478348.
Febbe added a comment.

Applied clang-format `QualifierAlignment: Left`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/clangd/TidyProvider.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,270 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable =(Copyable const &) = default;
+  Copyable =(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+// Replacements in expansions from macros or of their parameters are buggy, so we don't fix them.
+// Todo (future): The source location of macro parameters might be fixed in the future
+#define FUN(Mov) valueReceiver((Mov))
+void falseMacroExpansion() {
+  Movable Mov;
+  FUN(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Parameter 'Mov' is copied on last use, consider moving it instead. 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:215
"-bugprone-use-after-move",
+   // Using an CFG and might crash on invalid code:
+   "-performance-unnecessary-copy-on-last-use",

kadircet wrote:
> comment seems half-finished here. is there any other reason than trying to 
> analyze `CFG` for invalid code?
Thanks, the "and" does not belong here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 477997.
Febbe marked 3 inline comments as done.
Febbe added a comment.

Fixed typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/clangd/TidyProvider.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,270 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable =(Copyable const &) = default;
+  Copyable =(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+// Replacements in expansions from macros or of their parameters are buggy, so we don't fix them.
+// Todo (future): The source location of macro parameters might be fixed in the future
+#define FUN(Mov) valueReceiver((Mov))
+void falseMacroExpansion() {
+  Movable Mov;
+  FUN(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Parameter 'Mov' is copied on last use, consider moving it instead. 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3950722 , @kadircet wrote:

> another thing that i noticed is usage of east consts in the implementation 
> files. no one seem to have brought it up so far (at least none that i can 
> see).
> in theory there are no explicit guidelines about it in LLVM coding style, but 
> rest of the codebase uses west const convention. so i am not sure if straying 
> away from it here will make much sense.

I think this should be regulated / enforced via clang-format if it is relevant 
at all to somebody.
In terms of consistency, the east-const I use is more consistent. Not to the 
previous written code, but to the language.
It is also completely irrelevant, because a new programmer will not understand 
that `const T const *` is actually `T const*` and not `T const * const`. An 
experienced programmer can understand it well either way.

In my eyes it should therefore always be east-const and also be taught in such 
a way, since such irritations do not arise thereby at all.

When it is ok for you to keep the east-const I would like to leave it as is, 
because it is on top a lot of work to manually search and replace those 
occurrences.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe marked 2 inline comments as done.
Febbe added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:123
+  hasLHS(ignoringParenImpCasts(declRefExpr(equalsNode(DeclRef)),
+  Context);
+  return !Matches.empty();

Febbe wrote:
> njames93 wrote:
> > Matching over the entire context seems pretty and a huge drain on 
> > performance, would it not make sense to just match inside the function 
> > declaration.
> > Side note maybe a RecursiveASTVisitor would make more sense here in terms 
> > of performance.
> > 
> > Same goes for isInLambdaCapture.
> How can I reduce the Context?
> 
> Also, in which terms, the RecursiveASTVisitor would make more sense here?
Replaced with RecursiveASTVisitor.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp:1
+// RUN: %check_clang_tidy %s -std=c++17 
performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 
performance-unnecessary-copy-on-last-use %t

njames93 wrote:
> Running this check explicitly in c++11/17 implies you expect different 
> diagnostics, if so you can use the `--check-suffixes` flag to enable checking 
> configurations. Search for other tests which use that if you're unsure 
Not necessarily, I just want the check to work with c++11 and after the changes 
to the language after c++17 (e.g. guaranteed copy elision). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 477846.
Febbe marked an inline comment as done.
Febbe added a comment.
Herald added subscribers: kadircet, arphaman.

Replaces match clauses with `RecursiveASTVisistor`s

- doubled performance
- fixed also a bug in template spezialisations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/clangd/TidyProvider.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,270 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable =(Copyable const &) = default;
+  Copyable =(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+// Replacements in expansions from macros or of their parameters are buggy, so we don't fix them.
+// Todo (future): The source location of macro parameters might be fixed in the future
+#define FUN(Mov) valueReceiver((Mov))
+void 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 476757.
Febbe marked 3 inline comments as done and 3 inline comments as done.
Febbe added a comment.

Improved fixit suggestion.

- No duplicated replacements.

Removed replacements for macros, since they could be wrong somehow.
Made some helperfunction non-trailing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,269 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable =(Copyable const &) = default;
+  Copyable =(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+// Replacements in expansions from macros or of their parameters are buggy, so we don't fix them.
+// Todo (future): The source location of macro parameters might be fixed in the future
+#define FUN(Mov) valueReceiver((Mov))
+void falseMacroExpansion() {
+  

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:79
+
+static auto findDeclRefBlock(CFG const *TheCFG, DeclRefExpr const *DeclRef)
+-> FindDeclRefBlockReturn {

njames93 wrote:
> We generally avoid trailing return types.
Ok, I'll make them non-trailing. 
Personally, I find trailing return types more readable, and they always work 
like expected.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:123
+  hasLHS(ignoringParenImpCasts(declRefExpr(equalsNode(DeclRef)),
+  Context);
+  return !Matches.empty();

njames93 wrote:
> Matching over the entire context seems pretty and a huge drain on 
> performance, would it not make sense to just match inside the function 
> declaration.
> Side note maybe a RecursiveASTVisitor would make more sense here in terms of 
> performance.
> 
> Same goes for isInLambdaCapture.
How can I reduce the Context?

Also, in which terms, the RecursiveASTVisitor would make more sense here?



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:290
+
+if (isInLambdaCapture(Param, *Result.Context)) {
+  // Lambda captures should not be fixed.

njames93 wrote:
> What's the reason for this logic, they require a different fix -
> `x` => `x(std::move(x))` 
> And a checking langopts for c++14.
> 
> Or is this because of implicit captures?
Yes, all fixes require at least c++14.
Also, implicit captures, require a different fix. 

So this is for both.
Honestly, I was too lazy to also implement some sort of lambda capture builder, 
which handles all of this.







Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:30
+  UnnecessaryCopyOnLastUseCheck(StringRef Name, ClangTidyContext *Context);
+  UnnecessaryCopyOnLastUseCheck(UnnecessaryCopyOnLastUseCheck &&) = delete;
+  UnnecessaryCopyOnLastUseCheck(const UnnecessaryCopyOnLastUseCheck &) = 
delete;

njames93 wrote:
> We typically avoid defining all the special member functions for clang tidy 
> checks. The destructor typically only needs to be defined if it's definition 
> can't be inside the header file.
It can't be, because I forward declared CFG, to reduce compile time a bit.
If you prefer to include the CFG's definition in the header, I can do that.

Shall I still remove the deleted special member functions?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp:16
+
+struct Copyable {
+  Copyable() = default;

njames93 wrote:
> This struct appears to be unused and has exactly the same definition as 
> `Movable`.
No, `Movable` is not trivially capyable, because it does not define a default 
destructor, `Copyable` does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3923643 , @Skylion007 
wrote:

> The main false positive I also keep seeing is in pybind11 where it suggests 
> call an std::move() for an implicit conversion, but the target of the 
> implicit conversion does not have an rvalue. (In this case, we have a 
> py::object which inherits from py::handle, and is just py::handle with 
> ownership semantics. This allows implicit conversion to a reference at 
> anytime, and therefore std::move has no effect here except to complicate the 
> code a bit.

Can you post preferably a minimal reproducible example?
Alternatively, you could write me again the exact position of the false 
positive. Including the implicit conversion operator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3936944 , @firewave wrote:

> Here's another false positive:
>
>   #include 
>   
>   void f(const std::string&);
>   
>   int main() {
>   std::string s;
>   f(s.empty() ? "<>" : s);
>   }
>
>
>
>   input.cpp:7:26: warning: Parameter 's' is copied on last use, consider 
> moving it instead. [performance-unnecessary-copy-on-last-use]
>   f(s.empty() ? "<>" : s);
>^
>std::move( )

Actually, this is correct:

https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIAKykrgAyeAyYAHI%2BAEaYxCBmAOykAA6oCoRODB7evgGp6ZkCoeFRLLHxSbaY9o4CQgRMxAQ5Pn6BdpgOWQ1NBCWRMXEJyQqNza15HeP9YYPlw0kAlLaoXsTI7BzmAMxhyN5YANQmO25OY8SYrKfYJhoAgvcPl14OR24pJolWj0f/HxSEDG6BAIEuYWAR2YbCWRxAMMwEERcO%2BFhBYLQXgIJzOpzcJzMZjQDFe73MZlxbnx0NYmCpNIpJn8bgYTJ2VkSABFngCjgA/T4QVE/DEoNY4/GMolYMkECkMs60tiKglMllsomnTk8x68gFCz5HEljcwANmNSzR%2Br5/wICDwCgAtLdEbiucaAHRu6yEswmM0adkWE5/W3/MVYyV4pUUtApACeftVXp9MbVRKORGTUqV9sdLp22DT1NjWo1we%2BuoefJtgIgRvN5st1rD4fzztddPdRzFLFQADckchvXSrRy63zIxKUxT%2B0OFbmCSOS9L/RWtRO2yduc860xsagjqgUnEmERiKcuQ2UsaBKazBbkFbRQRQeLsbOifGk0wFBlgDZDd/QnXdtwPbMTzPC8rxvQkzWbZ80WnT8lz9ed6T/ACgNZSswKebcxQhIxlUwbU913LcCIeAdUDwdBjWIJhkE2FICDgk15UfY0UhfHU90eMIcRYJgwmFVsawBMZzzwZAjlogxHHoI5olQTwjiYHsCGILwyKovkjS%2BH4KSYJlKN%2BST/jrZAmJYzA2IgTTTgAMSOFIQEBa0iWiMyuXHCyqw4FZaE4fxeD8DgtFIVBOGpSxfQUNYNnpXYeFIAhNCClYAGsEkCEKOEkcLMuizheAUEANHSzKVjgWAkDQFgUjoOJyEoRrmvoeIDkMYAuA0AbSCwAdZMwAA1PBMAAdwAeVPCK0poWgCDiCqIGiErojCJoE04NLGrYQQZoYWhdsi3gsBEoxxHOoa8Cubohwq27MFULpsS2NKhJqEraDwaImOIBMPCwErtLwFg9qCvgDGABQJumubGChmRBBEMR2CkVH5CUNQSt0Lh9F6lBrGsfR/oqyAVhPOpSU4J0QSvUx4ssLhEiOJ0Zp2cqai6WmXAYdxPDaPQQjmMoKj0NIMlpyY/EJ6WigYAYJeGQnOm6eoZjlvQNdp3pmhVoZ4nV7XhbyU2%2BiNhYTZWRL1k2CRgtC4rbpijgjlUAAOM0nTNSRjQMEj%2Bs9DRQ6OCBcEIEhCR2Lgll4DLzqWHK8v0TgitICKovd8rKuq5PSDqxAQAlFJsTaiAOpa4gIjpThvd9/3A96o4Q7DqLMHwC96L0fg0dEcQsf7nGVHUW6CdIKamPc7hoYKsKs5K92ZuxcucVQKhPZ9v2A564OO/DhtUCamvY/jxOatTsxJE9ABOB/H6fp%2BzXTwrXZzsrbHzpOtFqmAS6vXeheSuTQ4bKEMDUIQCBUBTQWrwauiksgQPCLQaBsDs4IJPp1YYcNmApAUDAggpBEFxFXsIGBcCSpAOQA8YgcMv40IaPgCKvAR7oyHtIEeigx74z0EHYwpNLDk2iJTYU0U2JZGepzbmqwHaY1sG%2BMIKCoGUPgelK4n1eDTyYLPHgzsOCL0waVDg2A3rIEPMQbeTcA7ABYhHbSXgGDZThA2IRNgjhRwvOfBOBc/7X3yhnD%2BvBc7fyqr/LKb8zDBJMZfZOKwhzEAyM4SQQA


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3936944 , @firewave wrote:

> The crash is gone.
>
> The false positive with the `[m]` capture is still present with `-std=c++11`.

Does only a warning appear? Or also a fix? Currently, only a warning should 
appear, but this is not fixable in c++11.

> Here's another false positive:
>
>   #include 
>   
>   void f(const std::string&);
>   
>   int main() {
>   std::string s;
>   f(s.empty() ? "<>" : s);
>   }
>
>
>
>   input.cpp:7:26: warning: Parameter 's' is copied on last use, consider 
> moving it instead. [performance-unnecessary-copy-on-last-use]
>   f(s.empty() ? "<>" : s);
>^
>std::move( )

Thanks for the minimal examples. Need to look into the AST's.

> I still have another false positive with static variables but have not gotten 
> around reducing it yet.
>
> I posted the false negatives in the GitHub ticket as those do not relate to 
> getting this approved.

Also thanks, I've looked over them, seems like there is a problem with the 
detection in the CFG regarding self assignments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 475589.
Febbe added a comment.

fixed lamda detection


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,262 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable =(Copyable const &) = default;
+  Copyable =(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template
+using RemoveRefT = typename RemoveRef::type;
+
+template 
+void initSomething(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov); // no warning: Todo(bug): Is this a bug? Fix or explain.
+  Mov = RemoveRefT{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

I have a problem with lambdas capturing by copy implicitly (`[=]()...`). It 
seems like, that child nodes are not reachable via a MatchFinder unless they 
are spelled in source. I actually don't know how to prevent a fix elegantly: My 
proposed idea is, to check the source location of the capture list for each 
lambda and check, if the declRefExpr is part of it... . This is bug prone, and 
possibly slow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 475030.
Febbe marked 5 inline comments as done.
Febbe added a comment.

Fixed a lot of false positives:

- no move for no automatic storage duration
- no move for lambda captures


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,247 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable =(Copyable const &) = default;
+  Copyable =(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template
+using RemoveRefT = typename RemoveRef::type;
+
+template 
+void initSomething(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov); // no warning: Todo(bug): Is this a 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3920016 , @firewave wrote:

> Getting this false positive:
>
>   #include 
>   
>   extern std::string str()
>   {
>   std::string ret;
>   return ret.empty() ? ret : ret.substr(1);
>   }
>
>
>
>   input.cpp:6:23: warning: Parameter 'ret' is copied on last use, consider 
> moving it instead. [performance-unnecessary-copy-on-last-use]
>   return ret.empty() ? ret : ret.substr(1);
>^
>std::move( )
>
> Appears to be caused by the ternary since I also have it inline with a 
> function parameter in another case.

As I know, the compilers can't copy elide always on ternary operators. 
Therefore, I have also a test, suggesting it on return.




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst:50
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.

Eugene.Zelenko wrote:
> Could such option be shared between different checks? Or even rely on 
> `.clang-format`?
I just oriented myself at other similar checks, which introduced this. It uses 
both there:

```
UnnecessaryCopyOnLastUseCheck::UnnecessaryCopyOnLastUseCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
  Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM),
```

Do you mean, that I can just remove the description of the option, because it 
is taken from global options?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 474260.
Febbe added a comment.

Fixed segfaults due to asserts which were wrongly assumed to be always true


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,168 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template
+using RemoveRefT = typename RemoveRef::type;
+
+template 
+void initSomething(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov); // no warning: Todo(bug): Is this a bug? Fix or explain.
+  Mov = RemoveRefT{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' may be copied on last use, consider forwarding it instead. [performance-unnecessary-copy-on-last-use]
+}
+
+void initSomethingVal(Movable const& Mov) {
+  initSomething(Movable{Mov}); 
+}
+
+void 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3915526 , @Skylion007 
wrote:

> Okay, now I am getting what I believe to be segfaults:
>
>   #0 0x564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
>   #1 0x564383480464 SignalHandler(int) Signals.cpp:0:0
>   #2 0x7f7c275c9420 __restore_rt 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
>   #3 0x5643804b0ea5 
> clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult
>  const&) (.cold) UnnecessaryCopyOnLastUseCheck.cpp:0:0
>   #4 0x56438262bba1 clang::ast_matchers::internal::(anonymous 
> namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes
>  const&) ASTMatchFinder.cpp:0:0
>   #5 0x5643826818df 
> clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*)
>  (/home/aaron/git/llvm-project/build/bin/clang-tidy+0x2d258df)
>
> It worked before this PR, now it just crashes on a lot of real world 
> codebases including just trying to run it for a few files on LLVM's own 
> codebase.

I thought I had fixed it with my newest diff. 
The reason for the segfaults are, that I removed the `llvm::Expected` and 
replaced it with asserts. But there are cases, where the asserts can be false 
with regular (currently unhandled code).

I also found issues:

- a fixup is proposed even when the copy-parameter is not mentioned in the 
code. This is the case for capturing lambdas `... = [=]{...};` -> `... = 
[std::move(=)]{...};`
- a fixup is proposed for trivial types like std::pair


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 473819.
Febbe added a comment.

Added some tests. Code cleanup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,168 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template 
+struct RemoveRef{
+  using type = T;
+};
+
+template
+using RemoveRefT = typename RemoveRef::type;
+
+template 
+void initSomething(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov); // no warning: Todo(bug): Is this a bug? Fix or explain.
+  Mov = RemoveRefT{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' may be copied on last use, consider forwarding it instead. [performance-unnecessary-copy-on-last-use]
+}
+
+void initSomethingVal(Movable const& Mov) {
+  initSomething(Movable{Mov}); 
+}
+
+void initSomethingRValRef(Movable const& Mov) {
+  

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 473770.
Febbe added a comment.

Fixed lValueReference detection in matcher


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+/* 
+Todo (improvement):
+Currently the check does not find last usages of fields of local objects.
+
+struct MoveOwner{
+  Movable Mov{};
+};
+
+void testFieldMove(){
+  MoveOwner Owner{};
+  valueReceiver(Owner.Mov);
+  Owner.Mov = Movable{};
+  valueReceiver(Owner.Mov);
+  // DISABLED-CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Owner.Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // DISABLED-CHECK-FIXES: valueReceiver(std::move(Owner.Mov));
+}
+*/
+
+/*
+Todo (improvement):
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+NoMove Nmov = std::make_shared>(M);
+shareMutex(Nmov);
+I = 42;
+}
+*/
Index: clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-unnecessary-copy-on-last-use
+

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#3912243 , @Skylion007 
wrote:

> One other bug I found with this diff, is it seems to suggest calling 
> std::move() on function args that are references, despite the fact that 
> invalidating the reference to the input arg could be undesirable. For 
> instance take a function that takes in a reference to a String, assigns a new 
> value to the arg reference, and then returns the value of the reference. It 
> suggests calling std::move() on the arg ref when it is returned, which 
> invalidate the reference to the argument.

Oh, that's not good. Actually, it should not run on references, only  
`declRefExpr(hasType(qualType(unless(anyOf(

  isConstQualified(), referenceType(), pointerType() ,..)` are allowed to be 
parsed.

Did you run this check alone, without other checks?

Regarding the appearance of `std::move(std::move( var );`, was there a move 
already? Does it occur in a header file? In case of running clang-tidy with 
fixups in parallel over multiple sources, it can happen, that a header file is 
parsed twice at the same time, resulting in duplicate insertions.

And last but not least, can you provide a source snipped or AST for both 
appearances?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

So I finally had time to apply your feedback.




Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:141
+  assert(BlockElement.DeclRefBlock);
+  auto Block = BlockElement.DeclRefBlock->succs();
+  // No successing DeclRefExpr found, appending successors

njames93 wrote:
> replace auto with type here.
Unused, because inlined next line.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:236
+  for (auto  : BlockedTypes) {
+if (Param->getType().getAsString().find(Type) != std::string::npos) {
+  return; // The type is blocked

njames93 wrote:
> Isn't blocked types meant to be a regular expression?
It is also handled twice, therefor I just removed it.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:294
+  }
+  // Generate Diagnostic
+}

njames93 wrote:
> What's this comment for?
Removed, old Todo I have overseen.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:10
+
+#pragma once
+#ifndef 
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H

njames93 wrote:
> Remove this, the include guard is enough.
Yes, I thought, `#pragma once` is less bug prone and might be faster. 



Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:21
+namespace clang {
+namespace tidy::performance {
+

njames93 wrote:
> I have no preference either way, but can these namespaces all be nested or 
> none nested.
Had a forward declaration there, and added it again to reduce include 
dependencies.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp:3
+// RUN: %check_clang_tidy %s -std=c++11 
performance-unnecessary-copy-on-last-use %t
+#include 
+// CHECK-FIXES: #include 

njames93 wrote:
> We avoid using the standard library for tests, if you need to use anything 
> from it you have to reimplement the parts you need yourself.
Kept the `static_assert` as comment and removed the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 473508.
Febbe marked 19 inline comments as done.
Febbe added a comment.

Applied feedback

- replaced some auto types with the actual type
- added `IncludeStyle` to the options list in the documentation
- Added "Limitations" paragraph, describing known limitations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+/* 
+Todo (improvement):
+Currently the check does not find last usages of fields of local objects.
+
+struct MoveOwner{
+  Movable Mov{};
+};
+
+void testFieldMove(){
+  MoveOwner Owner{};
+  valueReceiver(Owner.Mov);
+  Owner.Mov = Movable{};
+  valueReceiver(Owner.Mov);
+  // DISABLED-CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Owner.Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // DISABLED-CHECK-FIXES: valueReceiver(std::move(Owner.Mov));
+}
+*/
+
+/*
+Todo (improvement):
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+NoMove Nmov = std::make_shared>(M);
+shareMutex(Nmov);
+I = 42;
+}
+*/
Index: clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-unnecessary-copy-on-last-use
+
+performance-unnecessary-copy-on-last-use
+
+

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

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

I applied the rest of your feedback. 
There are other usages of `auto` like `auto FoundUsage` which is a Usage for 
example. ~Shall I also replace those obvious cases?~

- I just checked the guidelines. Obvious cases can be auto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 472722.
Febbe added a comment.

Applied feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,87 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+#include 
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+/*
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+NoMove Nmov = std::make_shared>(M);
+shareMutex(Nmov);
+I = 42;
+}
+
+*/
Index: clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
@@ -0,0 +1,45 @@
+.. title:: clang-tidy - performance-unnecessary-copy-on-last-use
+
+performance-unnecessary-copy-on-last-use
+
+
+Finds variables of non-trivially-copyable types, that are used in a copy
+construction on their last usage and suggest to wrap the usage in a
+``std::move``. The usage just before an assignment is interpreted as last usage.
+
+Many programmers tend to forget ``std::move`` for variables that can be moved.
+Instead, the compiler creates implicit copies, which can significantly decrease
+performance.
+
+Example:
+
+.. code-block:: c++
+
+  void acceptor(std::vector v);
+  void Function() {
+std::vector v;
+v.emplace_back(20);
+// The warning will suggest passing this as a rvalue-reference
+acceptor(v);
+  }
+
+Options
+---
+
+.. option:: BlockedTypes
+
+   A semicolon-separated list of names of types allowed to be copied 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 472698.
Febbe added a comment.

Applied feedback,
Also added the documentation for the second option ``BlockedFunctions``


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,87 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+#include 
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+/*
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+NoMove Nmov = std::make_shared>(M);
+shareMutex(Nmov);
+I = 42;
+}
+
+*/
Index: clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
@@ -0,0 +1,45 @@
+.. title:: clang-tidy - performance-unnecessary-copy-on-last-use
+
+performance-unnecessary-copy-on-last-use
+
+
+Finds variables of non-trivially-copyable types, that are used in a copy
+construction on their last usage and suggest to wrap the usage in a
+``std::move``. The usage just before an assignment is interpreted as last usage.
+
+Many programmers tend to forget ``std::move`` for variables that can be moved.
+Instead, the compiler creates implicit copies, which can significantly decrease
+performance.
+
+Example:
+
+.. code-block:: c++
+
+  void acceptor(std::vector v);
+  void Function() {
+std::vector v;
+v.emplace_back(20);
+// The warning will suggest passing this as a rvalue-reference
+acceptor(v);
+  }
+
+Options
+---
+
+.. option:: BlockedTypes

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe marked 10 inline comments as done.
Febbe added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst:6
+
+Finds variables of non-trivially-copyable types, that are used in a copy 
construction on their last usage and suggest to wrap the usage in a `std::move`.
+The usage just before an assignment is interpreted as last usage.  

Eugene.Zelenko wrote:
> Please synchronize with Release Notes, follow 80 characters limit and use 
> double back-ticks for language constructs.
Do you mean, to add this short description to the release notes?
Is the 80 characters limit for the line, or the whole description?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 472458.
Febbe added a comment.

Added documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,89 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// NRmUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+#include 
+#include 
+#include 
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+/*
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+NoMove Nmov = std::make_shared>(M);
+shareMutex(Nmov);
+I = 42;
+}
+
+*/
Index: clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - performance-unnecessary-copy-on-last-use
+
+performance-unnecessary-copy-on-last-use
+==
+
+Finds variables of non-trivially-copyable types, that are used in a copy construction on their last usage and suggest to wrap the usage in a `std::move`.
+The usage just before an assignment is interpreted as last usage.  
+
+Many programmers tend to forget `std::move` for variables that can be moved. 
+Instead, the compiler creates implicit copies, which can significantly decrease performance.
+
+
+Example:
+
+.. code-block:: c++
+
+  void acceptor(std::vector v);
+  void Function() {
+std::vector v;
+v.emplace_back(20);
+// The warning will suggest pasing this as a rvalue-reference
+acceptor(v);
+  }  
+
+Options
+---
+
+.. option:: BlockedTypes
+
+   A semicolon-separated list of names of 

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

I also have to add the 
http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-copy-on-last-use.html
 page, but I don't have any clue how, and where to start.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

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

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check - finds occurences of last uses, which are copied. - suggests adding a `std::move`. - detects assignments as last

[Fabian Keßler via Phabricator via cfe-commits]

Febbe created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
Febbe requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137205

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp
@@ -0,0 +1,89 @@
+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// NRmUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+#include 
+#include 
+#include 
+// CHECK-FIXES: #include 
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable =(Movable const &) = default;
+  Movable =(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+static_assert(!std::is_trivially_copyable_v, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const );
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+Mov.memberUse();
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+Mov = Movable{};
+  } else {
+Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+/*
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+NoMove Nmov = std::make_shared>(M);
+shareMutex(Nmov);
+I = 42;
+}
+
+*/
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h
@@ -0,0 +1,56 @@
+//===--- UnnecessaryCopyOnLastUseCheck.h - clang-tidy-*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// Author: Fabian Keßler "Febbe" 
+//===--===//
+
+#pragma once
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+#include "clang/Analysis/CFG.h"
+#include "llvm/ADT/DenseMap.h"
+
+namespace clang {
+namespace tidy::performance {
+
+/// A check that flags value parameters on last usages that can safely be moved,
+/// because they are copied anyway.
+///
+/// For the user-facing 

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

@salman-javed-nz you're welcome, I only fixed it because I saw the bug in the 
trace directly. Normally, I would only fix C/C++ stuff :D.


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

https://reviews.llvm.org/D119481

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

@JonasToth yes, it would be nice, to test this and then push it for me. Also a 
backport to 14.0 would be good :).


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

https://reviews.llvm.org/D119481

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 411606.
Febbe added a comment.

Applied the feedback


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

https://reviews.llvm.org/D119481

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -64,13 +64,14 @@
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
-  result = './'
+  result = os.path.realpath('./')
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:
   print('Error: could not find compilation database.')
   sys.exit(1)
-result += '../'
-  return os.path.realpath(result)
+result = parent
+  return result
 
 
 def make_absolute(f, directory):


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -64,13 +64,14 @@
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
-  result = './'
+  result = os.path.realpath('./')
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:
   print('Error: could not find compilation database.')
   sys.exit(1)
-result += '../'
-  return os.path.realpath(result)
+result = parent
+  return result
 
 
 def make_absolute(f, directory):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

I don't think I have commit rights, so someone of you also have to commit this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119481

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

Currently, only tested on Windows. 
This should also increase performance, since the path is canonicalized only 
once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119481

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows find_compilation_database checked only for "/" as exit point, but on windows, this root is impossible. Fixes #53642

[Fabian Keßler via Phabricator via cfe-commits]

Febbe created this revision.
Herald added a subscriber: carlosgalvezp.
Febbe requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119481

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -64,13 +64,14 @@
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
-  result = './'
+  result = os.path.realpath('./')
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:
   print('Error: could not find compilation database.')
   sys.exit(1)
-result += '../'
-  return os.path.realpath(result)
+result = parent
+  return result
 
 
 def make_absolute(f, directory):


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -64,13 +64,14 @@
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
-  result = './'
+  result = os.path.realpath('./')
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:
   print('Error: could not find compilation database.')
   sys.exit(1)
-result += '../'
-  return os.path.realpath(result)
+result = parent
+  return result
 
 
 def make_absolute(f, directory):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

[Fabian Keßler via Phabricator via cfe-commits]

Febbe marked an inline comment as done.
Febbe added a comment.

Thank you for the review. I am done and you can commit the patch :) . I don't 
have the rights to commit.


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

https://reviews.llvm.org/D118847

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

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 407200.
Febbe marked 4 inline comments as done.
Febbe added a comment.

Last batch of suggested changes


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

https://reviews.llvm.org/D118847

Files:
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,49 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+struct strong_ordering {
+  using value_type = signed char;
+  static strong_ordering const less;
+  static strong_ordering const equal;
+  static strong_ordering const equivalent;
+  static strong_ordering const greater;
+
+  constexpr strong_ordering(value_type v) : val(v) {}
+  template 
+  requires(T{0}) friend constexpr auto
+  operator==(strong_ordering v, T u) noexcept -> bool {
+return v.val == u;
+  }
+  friend constexpr auto operator==(strong_ordering v, strong_ordering w) 
noexcept -> bool = default;
+
+  value_type val{};
+};
+inline constexpr strong_ordering strong_ordering::less{-1};
+inline constexpr strong_ordering strong_ordering::equal{0};
+inline constexpr strong_ordering strong_ordering::equivalent{0};
+inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+struct TestDefaultOperatorA {
+  int a{};
+  int b{};
+
+  friend auto operator<=>(const TestDefaultOperatorA &, const 
TestDefaultOperatorA &) noexcept = default;
+};
+
+struct TestDefaultOperatorB {
+  int a{};
+  int b{};
+  friend auto operator==(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept -> bool = default;
+  friend bool operator<(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
+  // CHECK-FIXES: {{^}}  friend auto operator<(const TestDefaultOperatorB &, 
const TestDefaultOperatorB &) noexcept -> bool = default;{{$}}
+};
Index: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
@@ -404,14 +404,17 @@
   const auto *Fr = Result.Nodes.getNodeAs("Friend");
   assert(F && "Matcher is expected to find only FunctionDecls");

-  if (F->getLocation().isInvalid())
+  // Three-way comparison operator<=> is syntactic sugar and generates implicit
+  // nodes for all other operators.
+  if (F->getLocation().isInvalid() || F->isImplicit())
 return;

-  // Skip functions which return just 'auto'.
+  // Skip functions which return 'auto' and defaulted operators.
   const auto *AT = F->getDeclaredReturnType()->getAs();
-  if (AT != nullptr && !AT->isConstrained() &&
-  AT->getKeyword() == AutoTypeKeyword::Auto &&
-  !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+  if (AT != nullptr &&
+  ((!AT->isConstrained() && AT->getKeyword() == AutoTypeKeyword::Auto &&
+!hasAnyNestedLocalQualifiers(F->getDeclaredReturnType())) ||
+   F->isDefaulted()))
 return;

   // TODO: implement those


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,49 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+struct strong_ordering {
+  using value_type = signed char;
+  static strong_ordering const less;
+  static strong_ordering const equal;
+  static strong_ordering const equivalent;
+  static strong_ordering const greater;
+
+  constexpr strong_ordering(value_type v) : val(v) {}
+  template 
+  requires(T{0}) friend constexpr auto
+  operator==(strong_ordering v, T u) 

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

Can I still add a diff, or does this cause a revoke (apply the rest of the 
feedback)? 
Also, is the commit added automatically to the repo, or do I / another one have 
to rebase it.

Sorry for those questions, this is my first contribution to llvm.


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

https://reviews.llvm.org/D118847

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

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp:71
+  template 
+  requires(T{0}) //
+  friend constexpr auto

kbobyrev wrote:
> nit: `//` looks unrelated
I used the `//` to break the line there (avoids // clang-format off/on)

in my opinion, it should look like this:

`template  requires(T{0}) \n`


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

https://reviews.llvm.org/D118847

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

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

[Fabian Keßler via Phabricator via cfe-commits]

Febbe updated this revision to Diff 407115.
Febbe retitled this revision from "Added early exit for defaulted FunctionDecls.
This prevents matching of defaulted comparison operators.   
fixes llvm#53355" to "Added early exit for defaulted FunctionDecls.".
Febbe edited the summary of this revision.
Febbe added a comment.

Applied feedback


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

https://reviews.llvm.org/D118847

Files:
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,50 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+struct strong_ordering {
+  using value_type = signed char;
+  static strong_ordering const less;
+  static strong_ordering const equal;
+  static strong_ordering const equivalent;
+  static strong_ordering const greater;
+
+  constexpr strong_ordering(value_type v) : val(v) {}
+  template 
+  requires(T{0}) //
+  friend constexpr auto
+  operator==(strong_ordering v, T u) noexcept -> bool {
+return v.val == u;
+  }
+  friend constexpr auto operator==(strong_ordering v, strong_ordering w) 
noexcept -> bool = default;
+
+  value_type val{};
+};
+inline constexpr strong_ordering strong_ordering::less{-1};
+inline constexpr strong_ordering strong_ordering::equal{0};
+inline constexpr strong_ordering strong_ordering::equivalent{0};
+inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+struct TestDefaultOperatorA {
+  int a{};
+  int b{};
+
+  friend auto operator<=>(const TestDefaultOperatorA &, const 
TestDefaultOperatorA &) noexcept = default;
+};
+
+struct TestDefaultOperatorB {
+  int a{};
+  int b{};
+  friend auto operator==(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept -> bool = default;
+  friend bool operator<(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
+  // CHECK-FIXES: {{^}}  friend auto operator<(const TestDefaultOperatorB &, 
const TestDefaultOperatorB &) noexcept -> bool = default;{{$}}
+};
Index: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
@@ -404,14 +404,17 @@
   const auto *Fr = Result.Nodes.getNodeAs("Friend");
   assert(F && "Matcher is expected to find only FunctionDecls");

-  if (F->getLocation().isInvalid())
+  // three-way comparison operator<=> is syntactic sugar and generates implicit
+  // nodes for all other operators
+  if (F->getLocation().isInvalid() || F->isImplicit())
 return;

-  // Skip functions which return just 'auto'.
+  // Skip functions which return 'auto' and defaulted operators.
   const auto *AT = F->getDeclaredReturnType()->getAs();
-  if (AT != nullptr && !AT->isConstrained() &&
-  AT->getKeyword() == AutoTypeKeyword::Auto &&
-  !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+  if (AT != nullptr &&
+  ((!AT->isConstrained() && AT->getKeyword() == AutoTypeKeyword::Auto &&
+!hasAnyNestedLocalQualifiers(F->getDeclaredReturnType())) ||
+   F->isDefaulted()))
 return;

   // TODO: implement those


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,50 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+struct strong_ordering {
+  using value_type = signed char;
+  static strong_ordering const less;
+  static strong_ordering const equal;
+  static 

[PATCH] D118847: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes llvm#53355

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

Push, would be nice to see this in the llvm-14 release. I can also do a review 
for someone else as a favor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118847

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

[PATCH] D118734: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes #53355

[Fabian Keßler via Phabricator via cfe-commits]

Febbe abandoned this revision.
Febbe added a comment.

Updated in https://reviews.llvm.org/D118847


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118734

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

[PATCH] D118847: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes llvm#53355

[Fabian Keßler via Phabricator via cfe-commits]

Febbe created this revision.
Herald added a subscriber: carlosgalvezp.
Febbe requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118847

Files:
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,50 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+  struct strong_ordering {
+using value_type = signed char;
+static strong_ordering const less;
+static strong_ordering const equal;
+static strong_ordering const equivalent;
+static strong_ordering const greater;
+
+constexpr strong_ordering(value_type v) : val(v) {}
+template 
+requires(T{0}) //
+friend constexpr auto
+operator==(strong_ordering v, T u) noexcept -> bool {
+  return v.val == u;
+}
+friend constexpr auto operator==(strong_ordering v, strong_ordering w) 
noexcept -> bool = default;
+
+value_type val{};
+  };
+  inline constexpr strong_ordering strong_ordering::less{-1};
+  inline constexpr strong_ordering strong_ordering::equal{0};
+  inline constexpr strong_ordering strong_ordering::equivalent{0};
+  inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+struct TestDefaultOperatorA {
+  int a{};
+  int b{};
+
+  friend auto operator<=>(const TestDefaultOperatorA &, const 
TestDefaultOperatorA &) noexcept = default;
+};
+
+struct TestDefaultOperatorB {
+  int a{};
+  int b{};
+  friend auto operator==(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept -> bool = default;
+  friend bool operator<(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
+  // CHECK-FIXES: {{^}}  friend auto operator<(const TestDefaultOperatorB &, 
const TestDefaultOperatorB &) noexcept -> bool = default;{{$}}
+};
Index: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
@@ -404,15 +404,18 @@
   const auto *Fr = Result.Nodes.getNodeAs("Friend");
   assert(F && "Matcher is expected to find only FunctionDecls");
 
-  if (F->getLocation().isInvalid())
+  // operator<=> generates also the AST of e.g. operator==, which is implicit
+  if (F->getLocation().isInvalid() || F->isImplicit())
 return;
 
-  // Skip functions which return just 'auto'.
-  const auto *AT = F->getDeclaredReturnType()->getAs();
-  if (AT != nullptr && !AT->isConstrained() &&
-  AT->getKeyword() == AutoTypeKeyword::Auto &&
-  !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
-return;
+  { // Skip functions which return just 'auto' or are also defaulted operators.
+const auto *AT = F->getDeclaredReturnType()->getAs();
+if (AT != nullptr &&
+((!AT->isConstrained() && AT->getKeyword() == AutoTypeKeyword::Auto &&
+  !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType())) ||
+ F->isDefaulted()))
+  return;
+  }
 
   // TODO: implement those
   if (F->getDeclaredReturnType()->isFunctionPointerType() ||


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,50 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+  struct strong_ordering {
+using value_type = signed char;
+static strong_ordering const less;
+static strong_ordering const equal;
+static 

[PATCH] D118734: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes #53355

[Fabian Keßler via Phabricator via cfe-commits]

Febbe created this revision.
Herald added a subscriber: carlosgalvezp.
Febbe requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118734

Files:
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,51 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+  struct strong_ordering {
+using value_type = signed char;
+static strong_ordering const less;
+static strong_ordering const equal;
+static strong_ordering const equivalent;
+static strong_ordering const greater;
+
+constexpr strong_ordering(value_type v) : val(v) {}
+template 
+requires(T{0}) //
+friend constexpr auto
+operator==(strong_ordering v, T u) noexcept -> bool {
+  return v.val == u;
+}
+
+friend constexpr auto operator==(strong_ordering v, strong_ordering w) 
noexcept -> bool = default;
+
+value_type val{};
+  };
+  inline constexpr strong_ordering strong_ordering::less{-1};
+  inline constexpr strong_ordering strong_ordering::equal{0};
+  inline constexpr strong_ordering strong_ordering::equivalent{0};
+  inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+struct TestDefaultOperatorA {
+  int a{};
+  int b{};
+
+  friend auto operator<=>(const TestDefaultOperatorA &, const 
TestDefaultOperatorA &) noexcept = default;
+};
+
+struct TestDefaultOperatorB {
+  int a{};
+  int b{};
+  friend auto operator==(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept -> bool = default;
+  friend bool operator<(const TestDefaultOperatorB &, const 
TestDefaultOperatorB &) noexcept = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use a trailing return type for 
this function [modernize-use-trailing-return-type]
+  // CHECK-FIXES: {{^}}  friend auto operator<(const TestDefaultOperatorB &, 
const TestDefaultOperatorB &) noexcept -> bool = default;{{$}}
+};
Index: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
@@ -404,14 +404,16 @@
   const auto *Fr = Result.Nodes.getNodeAs("Friend");
   assert(F && "Matcher is expected to find only FunctionDecls");
 
-  if (F->getLocation().isInvalid())
+  // operator<=> generates also the AST of e.g. operator==, which is implicit
+  if (F->getLocation().isInvalid() || F->isImplicit())
 return;
 
-  // Skip functions which return just 'auto'.
-  const auto *AT = F->getDeclaredReturnType()->getAs();
-  if (AT != nullptr && !AT->isConstrained() &&
-  AT->getKeyword() == AutoTypeKeyword::Auto &&
-  !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+  // Skip functions which return just 'auto' or are also defaulted operators.
+  if (const auto *AT = F->getDeclaredReturnType()->getAs();
+  AT != nullptr &&
+  ((!AT->isConstrained() && AT->getKeyword() == AutoTypeKeyword::Auto &&
+!hasAnyNestedLocalQualifiers(F->getDeclaredReturnType())) ||
+   F->isDefaulted()))
 return;
 
   // TODO: implement those


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
@@ -52,3 +52,51 @@
 T req2(T t) requires requires { t + t; };
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
   // CHECK-FIXES: {{^}}auto req2(T t) -> T requires requires { t + t; };{{$}}
+
+//
+// Operator c++20 defaulted comparison operators
+//
+// Requires 
+
+namespace std {
+  struct strong_ordering {
+using value_type = signed char;
+static strong_ordering const less;
+static strong_ordering const equal;
+static strong_ordering const equivalent;
+static strong_ordering const greater;
+
+constexpr