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

Mon Nov 28 00:31:40 PST 2022

kadircet added a comment.

In D137205#3951230 <https://reviews.llvm.org/D137205#3951230>, @Febbe wrote:

> In D137205#3950722 <https://reviews.llvm.org/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.

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

> In terms of consistency, the east-const I use is more consistent. Not to the previous written code, but to the language.

not sure what you mean by it's consistent "to the language", but right now even in your header files you have both east and west consts. probably to keep public interfaces consistent across the codebase, as you've noticed.
another consequence of this is anyone modifying this code later on, will need to make a choice between sticking to the code inside this file vs code inside the rest of the codebase, which will create more discussions with every patch making people lose time and moreover bring the file into an inconsistent state, as some of those discussions will insert west consts.

> 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 const`s are more readable and fool-proof. it's unfortunate that most of the C++ world is stuck with the `west const`s.

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

As mentioned above, I understand your opinion here and deep down wish that was the state. But as I explained before, in a code base where thousands of developers are contributing, I don't think we can afford everyone introducing code with their own opinions. Despite not explicitly having a style guide on east vs west consts, it's clear from the rest of the codebase that one should adhere to west consts.
So I'd be glad if you could take your time to replace east consts to west.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205