coin-or / Dip

DIP is a decomposition-based solver framework for mixed integer linear programs.
https://github.com/coin-or/Dip/wiki
Eclipse Public License 1.0
17 stars 7 forks source link

Use clang-format to beautify code #115

Open spoorendonk opened 4 years ago

spoorendonk commented 4 years ago

Make the code format look the same allover. https://clang.llvm.org/docs/ClangFormat.html Used in Cbc also.

tkralphs commented 4 years ago

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.

spoorendonk commented 4 years ago

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

spoorendonk commented 4 years ago

I would also suggest to put this in the CI files to early out on unformatted code

spoorendonk commented 4 years ago

About broken stuff. Are you referring to https://travis-ci.org/coin-or/Dip/jobs/574776145 The xcode8 build seems to have been broken for a while. Not sure what is going on there.

tkralphs commented 4 years ago

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.

spoorendonk commented 4 years ago

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
spoorendonk commented 4 years ago

I pushed an llvm version

spoorendonk commented 4 years ago

pr at #116

tkralphs commented 4 years ago

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.

spoorendonk commented 4 years ago

I am not sure if one should do a reformat or a refactor first? Overhaul the code in general before formatting, I mean. Practical question: Would formatting other branches not mean doing PR's to each of them?

tkralphs commented 4 years ago

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.