Hi David,
We don't break backwards compatability lightly. The last time was 12 years
ago. In that time, there has been some accumulation of cruft, and in
addition there are some necessary changes in order to fix underlying
problems.
Regarding the specific functions you mention, the IsElement() functions
are, as you say, convenience functions, some of which date back to the
original Stahl/Walters Babel. While GetAtomicNumber() == 1 is less readable
and error prone as you say, I have added the elements as enums,
OBElement::Hydrogen, to mitigate this. Furthermore, while the IsElement()
methods only covered a small number of elements, the ones where people are
most likely to know the atomic number (where's IsFluorine() for instance),
the OBElement namespace covers them all.
So why have I removed them? They are convenience functions trivially
implementable in terms of the remaining API. There is nothing to prevent a
user implementing the function themselves as a macro or whatever. From the
port of Open Babel to remove these function, I can give examples of where
the use of these functions results in inefficient code. Obviously there are
two function lookups instead of one. But also, I have seen constructions
like "if atom->IsHydrogen then this; else if atom->IsCarbon then this; and
so on. In other words, instead of getting the atomic number once and
switching on it, the maximum possible amount of work is done.
Regarding IsSingle(), the situation is more clear cut. This is a function
which appears to be (and is documented to be) synonymous with
GetBondOrder() == 1. Aha, but it is not, and only by looking at the source
code would you have realised the difference. It was a surprise to me, and I
think it is used synonymous with GetBondOrder==1 throughout the
codebase...but there's no way to tell what the caller intended. All we can
do is prevent nasty surprises for users in the future. This change was made
in combination with an overhaul of the treatment of kekulization and
aromaticity last year.
Regards,
- Noel
On 11 January 2018 at 21:26, David Koes <dk...@pitt.edu> wrote:
> I'd like to advocate for _not_ removing simple convenience functions like
> OBAtom->IsHydrogen, OBBond->IsSingle. I don't see any benefit: it breaks
> backwards compatibility, using GetAtomicNumber() == 1 is less readable and
> more error prone, and GetBondOrder() == 1 doesn't have the same semantics
> as IsSingle did.
>
> My two cents.
>
> David Koes
>
> Assistant Professor
> Computational & Systems Biology
> University of Pittsburgh
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> OpenBabel-Devel mailing list
> OpenBabel-Devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openbabel-devel
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
