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

Fix bug in ParameterHandler::parse_input for .prm #12789

Merged
merged 1 commit into from Oct 4, 2021

Conversation

sebproell
Copy link
Contributor

The entries_set_status was not updated when parsing from .prm files.

I am not super happy with my fix as it copies the lines from ParameterHandler::set into the .prm parsing logic. When parsing .xml and .json files, it seems to directly call the set method. This inconsistency probably lead to the bug in the first place. Maybe it would be a good idea to call set for the .prm case as well? I can do it in another PR if you agree.

The entries_set_status was not updated when parsing from .prm files.
@drwells
Copy link
Member

drwells commented Oct 4, 2021

Its good to fix bugs but yes, I agree - its not good that we have duplicated code. A better design would not have these separate maps which can get out of sync. I don't know how to do it off the top of my head but we really should have one internal representation that would guarantee consistency. Would you be willing to give that a try?

@sebproell
Copy link
Contributor Author

@drwells Sure. I don't see a big reason why we shouldn't use the set function directly in the scan_line function, which is consistent with the xml and json variants. I will look into it but suggest to merge this PR anyway, since it contains a tested fix for the problem?

Not sure I completely understand what you mean by

these separate maps which can get out of sync

Imo, the problem comes mainly from the duplicated logic of validating and setting values for different files.

@drwells
Copy link
Member

drwells commented Oct 4, 2021

I mean that ideally we would encode this information with the entries in the property tree itself, but I don't know how to do it - it might not be feasible to fundamentally alter the boost tree in that way.

@drwells
Copy link
Member

drwells commented Oct 4, 2021

/rebuild

@drwells drwells merged commit f37e029 into dealii:master Oct 4, 2021
@sebproell sebproell deleted the prm-parse-mandatory branch February 2, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants