Skip to content
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

Open
spoorendonk opened this issue Aug 21, 2019 · 11 comments
Open

Use clang-format to beautify code #115

spoorendonk opened this issue Aug 21, 2019 · 11 comments

Comments

@spoorendonk
Copy link
Contributor

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

@tkralphs
Copy link
Member

tkralphs commented Aug 21, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@spoorendonk
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

I pushed an llvm version

@spoorendonk
Copy link
Contributor Author

pr at #116

@tkralphs
Copy link
Member

tkralphs commented Apr 7, 2020

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
Copy link
Contributor Author

spoorendonk commented Apr 8, 2020

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants