Clean up tools/obabel.cpp#2152
Conversation
e-kwsm
commented
Apr 2, 2020
- clang-format
- use continue appropriately to reduce nesting levels
|
Sounds good but I can't review this patch, as it mixes whitespace changes with code changes. If you keep these in separate commits, then we see what's changed at each step, but when you merge them we haven't any idea. |
|
Separated into commits. |
|
Some of these commits involve syntax changes that change code that is working fine now into code that may only be supported by newer compilers. I'm not familiar enough with the details of every compiler version to know exactly when these features were introduced, and what version of gcc ships with what version of CentOS. Every time I see newer syntax in a PR I think to myself - well, how can I merge this if I don't know whether it will cause problems for people? Maybe @ghutchis has another take on this. |
|
I'm not sure about the the {} notation, but I'm willing to discuss that on-list. |
|
Curly braces for constructor calls are fine since at least C++11 and have some advantages: https://stackoverflow.com/questions/15396124/calling-constructor-with-braces |
|
Most patches seem OK at a glance, although it is difficult to overview the if statement change. As regards whitespace changes, in principle I support an automatic cleanup and uniformisation of the code, but the proposed whitespace format is not attractive in my eyes. It is too condensed. Below is an example from GROMACS that uses clang-format as well and which is much more readable IMHO. |
137bd66 to
0aa4248
Compare
5dd8241 to
704476a
Compare
704476a to
6b34eee
Compare
a3f71dd to
77eca0b
Compare