-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use clang-format to beautify code #115
Comments
I definitely do want to have a standard formatting, but we may need to negotiate a little on the specifics. I don't particularly like Cbc's formatting, it is a compromise with the other developers. Actually, I had already worked out a format using astyle with my Ph.D student. I think he even did do some reformatting, but maybe it was only in his fork. Let me poke around and see if I can find that. The commit that does the reformatting breaks stuff (like blame), so we need to be careful about when we commit it. |
I do not have any strong opinion about formatting one way or another as long as it is consistent across all files. I would suggest to use the default llvm format. I will look at what is broken... |
I would also suggest to put this in the CI files to early out on unformatted code |
About broken stuff. Are you referring to https://travis-ci.org/coin-or/Dip/jobs/574776145 |
Yes, definitely some format checking in CI, but I've discussed this with someone knowledgeable recently and it seems it is not so easy to automate. I'm open to suggestions. Is the llvm default what you have checked in? I can check it. For broken stuff, I just meant that after you reformat, things can become difficult that used to be easy if you are not careful. For example, merging may be completely broken. Also, using the "blame" function of git becomes difficult because the last change of almost every line of every file traces back to the reformat. I don't know if there is a way to avoid this. We just want to be careful before committing mass changes for the purpose of reformatting. |
Got it. Yes it would be a pain to look back in time after formatting. It was the cbc version I checked in. For llvm it is simply find ./ -iname '*.h' -o -iname '*.cpp' -exec clang-format -style=llvm -i {} + no need for a .clang-format file for checking if formatting is needed in the CI pipeline one can do find ./ -iname '*.h' -o -iname '*.cpp' -exec clang-format -style=llvm -output-replacements-xml {} + | grep -c "<replacement " | grep 0 >/dev/null |
I pushed an |
pr at #116 |
Sorry for the delay. I am working a bit on DIP these days, but bandwidth is limited as usual. Will take a look. Would be nice to have some of the other branches also reformatted so as to ensure we can still merge down the line if we ever get to it. |
I am not sure if one should do a reformat or a refactor first? Overhaul the code in general before formatting, I mean. |
Good question. I guess it would be less risky to refactor first, possibly merging in things from other branches as needed. I'm not 100% sure what would happen if you reformat two branches with a common parent separately and then try to merge them back together. I guess it would be fine, but who knows... So yes, we would need to reformat each branch, but I guess we wouldn't necessarily need a PR for each if I just did it locally. There aren't actually that many branches we'd need to deal with. A couple of oild stables (or maybe just the current stable) and a couple of branches @jiadongwang was working in. |
Make the code format look the same allover. https://clang.llvm.org/docs/ClangFormat.html
Used in Cbc also.
The text was updated successfully, but these errors were encountered: