I'm familiar with PVS Studio, I've run both PVS Studio and Cppcheck over the Exodus codebase in the past. They can be a very useful tool and help to identify errors. I have no problems with using them, and I can see you're not doing it blindly. My problem is more about the fork and commit process, which I'll cover a bit below.Dr. MefistO wrote:Nemesis, I'm using PVS Studio to detect that bugs, that I've commited. You can ask me for it at PM. Then you will see what my every change means.
You need to treat a fork as a branch. A branch should be a staging area for a single task or job. Fixing the .rc include references is a single job. That should have its own fork, be reviewed, and merged to the main repository separately. Your pass over the code with CodeMaid should be a separate fork which receives a separate review, and in that case, I can tell you that I would be rejecting the CodeMaid changes. I would accept the .rc changes though. By bundling them together in the one fork, I can't do that, so I have to reject the whole thing.I know that I've comitted too many items, but what I have to do if it really valuable changes?)
The commit history of the repository is just as important as the current state of the repository. Changes need to be compartmentalized and tracked properly. You can work on more than one task at a time, but the changes need to be isolated from each other until they're complete, and ready for merging to the "mainline", at which point they need to be able to be merged separately. I'll create a ticket in the Jira system for your fork when I'm merging it, and use that ticket number in the merge commit, so that there's some proper historical tracking of the change, and a write-up on where it comes from and why it was done.
I've got a lot of experience with this kind of thing, and believe me, mega-changes are bad news, and cause big headaches. When bugs are introduced by changes, which they inevitably are, it needs to be easy to trace back through the commit history for a set of files, or the repository as a whole, and observe the relevant changes that have occurred, separating each job from each other and testing as you go to narrow down the changeset that introduced the problem. If you have a mega-change that re-formats all the files in the codebase, it introduces massive "white noise" into the diffs, which makes narrowing down the cause of the issue a nightmare, especially when that code change mixes reformatting with functional changes. If a change is formatting only, you know you can totally discount it, and do your diffs "around it", but when functional changes are mixed in, it becomes a pain point for years to come, and that commit will become the "usual suspect" for introduced errors. You've also created a bit of a merging nightmare for yourself, because I'm making my own code changes independently on files you've modified, and what's worse, the code formatting change you tried to do conflicts with my changes, so if you wanted to maintain your formatting changes, you're going to have to edit my changes and format them too in order to merge.
Sometimes large single changes are necessary, but they need to be coordinated and carefully managed, and they need to be done for a good reason.
The code currently is uniform, CodeMaid is just imposing a different style entirely. The etiquette for dealing with these kind of style issues is simple: "When in Rome....". Different areas of code can use different code styling, but files and libraries should be consistent within themselves. People making changes in a source file with 2000 lines should follow the formatting styles of the other 2000 lines, not write 5 lines using a different convention. That's the kind of thing I'd manage at the merge step, so I'd be keeping style issues in check there anyway. I wouldn't impose a single set of requirements on entirely new plugins or components though. As long as they're consistent in themselves, it doesn't cause a problem. This is C++ we're talking about, there are 1000 different styles, and no accepted standard formatting conventions.CodeMaid is not so bad, as you think: code is more readable, comments are still there, and so on..) And this is for uniformity of your code and code by others. I don't know how to explain. But to develope code by few people in one style it should be code in one style.=)
That the new code is "more readable" is also completely subjective. You may believe the code has become more readable for you, but there's no way you've looked at every file you just changed, and people will disagree on what's more readable even in the simple cases. I also know from experience that for every formatting convention, there's the "exception to the rule" cases, most especially with comments, and code formatting tools don't understand that. I looked at a bunch of files and cases in that commit that I knew CodeMaid would screw up, and it has. Lots of comments are using fixed layout and spacing. Static lookup tables are similarly being laid out using deliberate spacing to align columns and make them easier to view, and errors easier to spot. CodeMaid is wrecking these.
You don't change convention across a codebase on a whim without careful thought and review, and especially not sandwiched between several other jobs in a fork. If your fork introduces an error, and I want to do a diff to find the point where it was introduced, it'll be useless, because CodeMaid has changed half the lines of code in every file in the repository. There needs to be a very good reason to reformat code like this, and if it is to be done at all, it needs to be carefully planned, tested to exhaustion before it's done "for real", and when it is done for real, it needs to be done quickly, merged quickly, and coordinated with everyone who may be working with that codebase.
The CodeMaid changes will not be merged. Since that was the second commit, and all your changes after that are based on the reformatted code, your fork is poisoned. I'd suggest you create clean forks, one for each logical set of changes. Your .rc changes should be in one, the bugfixes in another, and your performance changes in another. Both the bugfix and performance changes you've made have some errors that need correcting, so once they're isolated into their own forks, I can review those properly and discuss them with you.I will update my pull request with some new commits, and you can check them as accurately as you want.
I hope I don't put you off making changes with my comments. I do appreciate your efforts to make improvements, we'll just need to take things a little slow here and get the hang of the fork, review, and merge process, and discuss any large-scale changes that touch a lot of files before the changes are made, because there might be other changes in development, or future plans, that would affect that job. I've also never done this on bitbucket before, so learning their system is a task in itself.
If you can start with a clean fork for just the .rc file changes, I'll try adn get the CLA up so that I can actually accept your changes, and we'll try and go through the review and merge process on that fork. With the first fork completed and merged, it'll clarify the process for me too, and I should be able to write a document on the process to help other people too.
