sbmlteam / libsbml

LibSBML is a native library for reading, writing and manipulating files and data streams containing the Systems Biology Markup Language (SBML). It offers language bindings for C, C++, C#, Java, JavaScript, MATLAB, Perl, PHP, Python, R and Ruby.
https://sbml.org/software/libsbml
Other
41 stars 28 forks source link

XMLNode_getPrefix unexpectedly returns NULL when prefix is absent #390

Closed eltix closed 2 months ago

eltix commented 3 months ago

Issue description

This is the documentation of the XMLNode_getPrefix() function. If the XML node has no prefix, e.g. as in this annotation node <COPASI xmlns="http://www.copasi.org/static/sbml">, the function allegedly returns the empty string.

/**
 * Returns the namespace prefix of this XML element.
 *
 * @param node XMLNode_t structure to be queried.
 *
 * @return the namespace prefix of this XML element.
 *
 * @note If no prefix
 * exists, an empty string will be return.
 *
 * @memberof XMLNode_t
 */
LIBLAX_EXTERN
const char *
XMLNode_getPrefix (const XMLNode_t *node);

However the implementation contradicts the documentation:

const char *
XMLNode_getPrefix (const XMLNode_t *node)
{
  if (node == NULL) return NULL;
  return node->getPrefix().empty() ? NULL : node->getPrefix().c_str();
}

Indeed, the function returns the null pointer instead. Therefore, either the documentation or the implementation should be amended. If the option of keeping the current implementation is preferred, then it would be nice to add a

bool XMLNode_hasPrefix(const XMLNode_t *node)

function (or could be called isSetPrefix) which would allow to check whether prefix is set and mention that function in the documentation like it is done for XMLNode_getAttrName for instance

fbergmann commented 3 months ago

would testing for NULL not be easier?

I agree that we have an issue with documentation here, and the documentation for the C-API should be changed to reflect, that a NULL ptr is returned. I just dont want to change the behavior of the library at this point when this part of the code has essentially been there for the past 20 years.

@skeating what do you think?

eltix commented 3 months ago

@fbergmann I am fine with returning NULL too. But then adding a hasPrefix() or isSetPrefix() function would be nice :) I've edited the issue description.

fbergmann commented 3 months ago

agreed, adding such a function can be done.