Skip to content

Clean up tools/obabel.cpp#2152

Open
e-kwsm wants to merge 8 commits into
openbabel:masterfrom
e-kwsm:cleanup-obabel
Open

Clean up tools/obabel.cpp#2152
e-kwsm wants to merge 8 commits into
openbabel:masterfrom
e-kwsm:cleanup-obabel

Conversation

@e-kwsm
Copy link
Copy Markdown
Contributor

@e-kwsm e-kwsm commented Apr 2, 2020

  • clang-format
  • use continue appropriately to reduce nesting levels

@baoilleach
Copy link
Copy Markdown
Member

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.

@e-kwsm
Copy link
Copy Markdown
Contributor Author

e-kwsm commented Apr 6, 2020

Separated into commits.

@fredrikw fredrikw mentioned this pull request Apr 7, 2020
1 task
@baoilleach
Copy link
Copy Markdown
Member

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.

@ghutchis
Copy link
Copy Markdown
Member

ghutchis commented Apr 8, 2020

I'm not sure about the the {} notation, but I'm willing to discuss that on-list.

@e-kwsm e-kwsm force-pushed the cleanup-obabel branch from e4b4ef4 to e64c553 Compare May 8, 2020 19:43
@dspoel
Copy link
Copy Markdown
Contributor

dspoel commented Sep 28, 2020

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

@dspoel
Copy link
Copy Markdown
Contributor

dspoel commented Sep 28, 2020

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.

bool SelectionData::initCoveredFraction(e_coverfrac_t type)
{
    coveredFractionType_ = type;
    if (type == CFRAC_NONE)
    {
        bDynamicCoveredFraction_ = false;
    }
    else if (!_gmx_selelem_can_estimate_cover(rootElement()))
    {
        coveredFractionType_     = CFRAC_NONE;
        bDynamicCoveredFraction_ = false;
    }
    else
    {
        bDynamicCoveredFraction_ = true;
    }
    coveredFraction_        = bDynamicCoveredFraction_ ? 0.0 : 1.0;
    averageCoveredFraction_ = coveredFraction_;
    return type == CFRAC_NONE || coveredFractionType_ != CFRAC_NONE;
}

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 this pull request may close these issues.

4 participants