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

add a C api for SBMLErrorLog #342

Merged
merged 3 commits into from
Aug 29, 2023
Merged

add a C api for SBMLErrorLog #342

merged 3 commits into from
Aug 29, 2023

Conversation

skeating
Copy link
Member

@skeating skeating commented Aug 19, 2023

Description

adds a c api for SBMLErrorLog

Motivation and Context

Fixes #329

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Change in documentation

Checklist:

  • I have updated all documentation necessary.
  • I have checked spelling in (new) comments.

Testing

  • Testing is done automatically and codecov shows test coverage
  • This cannot be tested automatically

{
return static_cast<const SBMLError*>(*it);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks scary, is this really needed? Is it safe? because those could always be XMLErrors and not SBMLErrors, so when accessing them from the C API we'd be giving them bad pointers. It would be better to just use the get error function returning an XMLError and have a function to figure out whether an XMLError is an SBMLError (which would be clear from the error code)

I'm sure I could devise a way to make that static cast do bad things if needed. So let's think about this once again. (sorry)

if (log == NULL)
return NULL;

return (const SBMLError_t*)(XMLErrorLog_getError((const XMLErrorLog_t*)(log), n));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still return a dynamic cast to an SBMLError, since the error returned might not be an SBMLError. At least we should have a test for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok reverted my static casts

SBMLErrorLog_free(log);
}
END_TEST

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need tests that test retrieving SBMLErrors when they are XMLErrors ensuring we don't garble ptrs.

Copy link
Member

@fbergmann fbergmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello Sarah, I added some remarks, could you just verify that I'm totally wrong on those. thank you.

@skeating
Copy link
Member Author

okay I changed the static cast and added test for the different types of error. I must admit I'm a little bit confused by the results

Copy link
Member

@fbergmann fbergmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for these changes, this looks great

@fbergmann fbergmann merged commit 6e069c0 into development Aug 29, 2023
33 of 35 checks passed
@fbergmann fbergmann deleted the c-api-additions branch August 29, 2023 09:46
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.

Give access to document error log in C API
2 participants