Tidy again #908
Tidy again #908
Conversation
|
Is it possible to rebase this PR on #906? I see there are lots of potential conflicts between these two. I will give the review later. |
|
Sorry, I did not see your PR before writing my own. I still think that enforcing checks for new code only is not particularly fair, as this new code is often based on preexisting code. Other than that, I think our PRs are mostly orthogonal. In this PR, I spent quite a bit of time to fix that one check everywhere. Found some errors where |
I could think about only three scenarios when the new code is often based on preexisting code:
For the first and second, the preexisting code won't considered as the newly edited or added lines, which won't be checked by clang-tidy. So let's consider the third scenario: copy and paste. First, "copy and paste" is generally considered evil, not only in C++, but in most programming languages as well. People shouldn't do this in the first place. However, you may argue that in this case, clang-tidy should give the warning about the "duplication", rather than warnings related to preexisting code base. I totally agree with this. Clang-tidy is only a tool and in my opinion, not a perfect tool. We use the tool to perform a specific task. And in this case, duplication detection isn't something clang-tidy could do. What could be a possible solution is we could search a CI tool for the duplication detection (Codacy can do this. So it must be possible in some ways). In all cases, just don't do copy and paste. There are tons of better alternatives.
As I have pointed out in #906, if you need to setup a clang-tidy configuration for a detector (in this case, neuland), you need to have .clang-tidy for three different folders and they should also be synced. Therefore, I would suggest to create a common file in |
|
@YanzhaoW, I will accept this PR and then update it according to your needs for neuland, ... |
|
Please, squash the commits for merging it. |
…l clang-tidy with coreguidelines-pro-type-cstyle-cast only, fixed that in the existing codebase (except for neuland and califa), again. +clang-format-15 changes +Fixed clang-tidy warnings.
|
Hi, @jose-luis-rs I've have updated neuland config in #906, should we merge that first? |
|
@jose-luis-rs: I have squashed the commits. Let's just hope that posterity will judge me by git diff -w. @YanzhaoW: Copying and pasting whole classes actually happens a lot. Besides you and me, nobody really thinks it is an issue. CI is also not a good stage to detect duplication, preferably we would notice this in the design phase. However, this is also an issue when reusing single lines from another file, which I consider legitimate. All the song and dance to be able to read from a tclonesarray is not something I would expect anyone to learn by heart. Traditionally, these lines contained a lot of c-style casts. If I had simply forbidden new code with such casts, the experience for the contributors would have been terrible: they follow established precedent and get punished for it. The majority of recent PhD students never submit any PR at all. Having overcome C++, conquered cmake, defeated git and beaten the github workflow into submission these few daring survivors now face the double boss fight of code reviews by both of us. If we also subject them to a barrage of "unfair" clang-tidy warnings about keywords like override which they likely have not encountered in our codebase or about them having the audacity to use variable indices, that will drive the number of new contributors towards zero. I am not advocating as treating all PRs as equally valuable in their own special way and everyone getting an award for participation, the goal is to keep the code working and somewhat maintainable. If I feel a PR will make our code base worse, I will say so plainly. But I also don't advocate biting off the head of new contributors on general principle, and selectively enforcing standards the rest of the code does not follow (and which they had no idea they should follow) against would be like that. |
|
Ok, if you think that's a better way, we could just do like that. But do you also think we could impose those restrictions on shared files in r3bbase such as FileSource, EventHeader or UcesbReader? I don't think most of people will ever contribute anything to that. Submitting a change to those files already requires some knowledge of programming. Maybe it won't be problem if we raise the standard high? |
|
Don't worry! for those files I also think that it is better |
Moved @YanzhaoW's .clang-tidy to neuland, created a global clang-tidy with coreguidelines-pro-type-cstyle-cast only, fixed that in the existing codebase (except for neuland and califa), again.
This PR is provided in the spirit of disaster relief and not as an endorsement of the project (which ended at the latest with the merging of #771). #771 is also the reason I did not fix CALIFA code, as there already exists a clustering without c-style casts. Neuland is the responsibility of @YanzhaoW, whom I consider fully capable of fixing any casting bugs. (Also, I did not want to wade through the tons of other warnings.)
I would request that anyone adding tidy checks to CI first resolves them in the preexisting codebase. Having a github action (or workflow or whatever Microsoft calls their stuff) to print yellow warnings for other checks is acceptable, holding new code to standards which our codebase fails is not a solution.
Edit: seems like github fails to CI my latest commit. Will try again tomorrow.