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

Handle temporary string xml attributes #24

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

peterNordin
Copy link
Collaborator

@peterNordin peterNordin commented Nov 16, 2022

Example on handling temp xml string attributes differently

@peterNordin peterNordin changed the title Work in progress Handle temoprary string xml attributes Nov 16, 2022
@peterNordin peterNordin changed the title Handle temoprary string xml attributes Handle temporary string xml attributes Nov 16, 2022
@peterNordin
Copy link
Collaborator Author

@robbr48 This "fixes" causality attribute handling, but there are other attributes being read similarly (but differently) that should be looked over.

I realized that these strings are temporary and only used to select which enum value (or similar) to set. Maybe instead of duplicating the strings (and having to deal with freeing them) we could make an additional ezxml get attribute function that returns the const char* (or null ptr) to the attribute string, and use that directly for comparison instead.

@robbr48
Copy link
Owner

robbr48 commented Nov 17, 2022

I'm not really sure what problem this fixes? If parseStringAttributeEzXml fails, the string reference is not modified anyway. The cleanest solution would perhaps be to supply a default value parameter? But C does not allow default values for parameters, so then we would always have to use it everywhere.

@peterNordin
Copy link
Collaborator Author

peterNordin commented Nov 17, 2022

The problem is that if parseStringAttributeEzXml fails, causality will point to a string not allocated on the heap, and then we can not free it. But if it succeeds, then it will instead point to a heap allocated (I assume _strdup is heap allocated) and we must free it to avoid a leak.

My "fix" (I dont like it) is to always ensure _strdup has been called, so that we always need to free it. Instead of having a situation where we must have logic that decides if we should free it or not.

But in this particular case where we do not even want to keep the causality string, why do we even need to _strdupe it?
We could instead "get it" from the XML representation as a const char* pointing into the XML representation and use it read only. But then we need a slightly different getStringAttributeEzXml function.

In fact, the more I think about it, maybe we should replace the parseStringAttributeEzXml function with getStringAttributeEzXml. Because the XML is already a string. Then call _strdup manually in those cases where it is needed, where we want to keep a copy of the string.

@peterNordin
Copy link
Collaborator Author

I made a new attempt at a slightly better solution.
Set the default value first, and only overwrite based on the string attribute if parseStringAttributeEzXml succeeds.
I check the const char* but the return value from the function could be used instead.

@robbr48
Copy link
Owner

robbr48 commented Nov 18, 2022

Ok, I see the problem and you solution is reasonable. You have a good point that we don't need to duplicate the string if we don't want to keep it. I want all the XML utility functions to be harmonized though, so if we change one we should change all (or add separate functions for different behaviour).

@peterNordin
Copy link
Collaborator Author

I guess my updated solution is a reasonable middle ground, then we do not need to add additional xml parsing functions.
I will look over the other attributes being parsed, and solve it in the same way.

@peterNordin peterNordin force-pushed the handling_of_temp_string_xml_attributes branch from 411ac8e to f2108cd Compare November 21, 2022 22:09
@peterNordin peterNordin marked this pull request as ready for review November 21, 2022 22:09
@peterNordin
Copy link
Collaborator Author

@robbr48 Ready for review, there are some TODOs in the code that I need input on. Essentially missing default values or handling about unknown values. Also look extra closely on the "clocks" handling, I am not sure if it still works as it is supposed to. The changes are mostly in the start and end of each changed code block.

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.

2 participants