icecc / icecream

Distributed compiler with a central scheduler to share build load
GNU General Public License v2.0
1.58k stars 250 forks source link

Low hanging fruits of some auto fixable improvements via clang-tidy #519

Closed AlexanderLanin closed 4 years ago

AlexanderLanin commented 4 years ago

not ready to be merged

What do you think? I could extend and clean this up if this goes to a reasonable direction.

llunak commented 4 years ago

I like the the modernize-use-null and modernize-redundant-void-arg, I think they're simple and obvious. The .gitignore is trivial too. I don't like the readability-simplify-boolean-expr one, although the code is technically the sema, semantically the code is different (there are many blocks of code that check many special cases and then return the "normal" return value at the end, so the code paths are not equal). I like the general idea of modernize-redundant-void-arg, but I'm not a big fan of "auto", I think it makes code less readable by using a "magic" type. Can clang-tidy be told to spell out the type of the loop variable?

llunak commented 4 years ago

Sorry for the question, but as it costs time to do so... would anyone actually merge this if I were to manually improve this PR?

Yes. Looking at the diff, code such as "for( size_t i = 0; ... cmake_checks[ i ] ..." already does not say what the type of the item is, so there is no regression when using auto, but changing code like "for( list<CompileServer *>::iterator it = " loses the type. So if it would be a lot of work to change all of them, please at least change those where the type already is spelled out now, that should be fairly simple to do just by looking at the diff.

Also please have a look at the Codacy automated review and the warnings it points out.

AlexanderLanin commented 4 years ago

The Codacy warning are legitimate, however I have not touched the issues it warns about and I don't know in which direction to fix them. The other jobs fail because of some wrong repository. This doesn't seem connect to this PR.

llunak commented 4 years ago

You're right about the Codacy warnings, I'll fix them.

llunak commented 4 years ago

Are you still working on this, or do you consider this ready for submission? If the latter, can you please rewrite the history to drop the unwanted commits instead of adding another commit reverting them?

AlexanderLanin commented 4 years ago

I'm considering this ready for review.

However I disagree with the rewrite as the first commit is clang-tidy applied as is and the second is about changing (reverting) one very particular case of that rewrite. It's not a drop of the commit, only of one instance. It makes sense to keep that separate, as merging them would indicate that clang-tidy did not change this one instance for no particular reason.

AlexanderLanin commented 4 years ago

Merging was not trivial, so I just reapplied clang-tidy with some cherry-picking.

llunak commented 4 years ago

Ok. Thank you.