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.

Sign up for GitHub

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

Jump to bottom

Refactoring; splitting up modelDescription & terminalsAndIcons parsing #136

Merged
merged 27 commits into from
Aug 8, 2024
Merged

Refactoring; splitting up modelDescription & terminalsAndIcons parsing #136

merged 27 commits into from
Aug 8, 2024

Conversation

PeterMeisrimelModelon
Copy link
Contributor

@PeterMeisrimelModelon PeterMeisrimelModelon commented Aug 7, 2024 •
edited
Loading

Refactoring:

  1. Removed common (xml scheme) attributes list, instead duplicated for terminalsAndIcons (for simplicity)
  2. Split all scheme related enums, global maps into modelDescription, terminalsAndIcons
  3. Replaced element/attr enums by appropriate unions. Introduced new macros for easy allocation: FMI3_ATTR_ANY for xml independent contexts, FMI_ATTR_TERMICON for terminalsAndIcons, FMI3_ATTR for modelDescription (FMI3_ATTR_MODELDESCRIPTION would artificially blow up lines and the 3 implicitly points to FMI3 modelDescription)
  4. Added fmi3_xml_type enum for context struct
  5. All accesses to global structs are now via functions and use appropriate switch (xmlType) case ...

Next step:

  • Re-location of code into more suitable logical file structures
  • Clean-up of imports

Peter Meisrimel added 21 commits August 1, 2024 16:36
work in progress; testing possible approaching for separating modelDe…
5215d40 
…scription and terminalsAndIcons parsing
Adding xmlType enum to context; lots of renaming for better distincti…
7522d44 
…on of different enums, maps, etc
Replacing direct accesses of global variables (maps) via function calls
dfc7d40
Replace more global maps via function accesses + making the global li…
ae9ddee 
…sts/maps const; adding lots of TODOs
Adding some intermitted duplication for refactoring
e6f0a32
prep
e56b211
work in progress; replaced fmi3_xml_attr_enu
44e63c0
work in progress; switch to fmi3_xml_scheme_info_t
9af9b21
work in progress; rename element handle maps
307ec7f
Separating maps 'constructors'
408eb79
tentative successful separation
773d292
duplicating common attributes
cf1a33e
Replacing attr enum by union
837ab1e
Replaced element enum with union
e58345c
Renaming for better type names
3753cb1
Fixing some messed up replaces
29f177f
Some cleanup; fixing a number of TODOs
b7b8721
Some cleanup; more clarity when using enum = -1 none
a045933
Some enum renaming for consistency
0dd0229
various re-naming for consistency
0f2a8f5
Misc cleanup
8ed041d
@PeterMeisrimelModelon PeterMeisrimelModelon self-assigned this Aug 7, 2024
@PeterMeisrimelModelon PeterMeisrimelModelon marked this pull request as ready for review August 7, 2024 09:25
modelonrobinandersson
modelonrobinandersson approved these changes Aug 7, 2024
View reviewed changes
Copy link
Collaborator

@modelonrobinandersson modelonrobinandersson left a comment

Choose a reason for hiding this comment

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

I think this all looks very good, well done!
I only noticed some very minor things (feel free to ignore).

src/XML/src/FMI3/fmi3_xml_parser.c Show resolved Hide resolved
src/XML/src/FMI3/fmi3_xml_parser.c Outdated Show resolved Hide resolved
src/XML/src/FMI3/fmi3_xml_parser.c Outdated Show resolved Hide resolved
src/XML/src/FMI3/fmi3_xml_parser.c Outdated Show resolved Hide resolved
src/XML/src/FMI3/fmi3_xml_parser_context_impl.h Outdated Show resolved Hide resolved
@PeterMeisrimelModelon @modelonrobinandersson
Apply suggestions from code review
1c33d6e 
Co-authored-by: Robin Andersson <robin.andersson@modelon.com>
@PeterMeisrimelModelon
Copy link
Contributor Author

@modelonrobinandersson See latest commit for a few more things renamed for clarity/consistency.

@modelonrobinandersson
Copy link
Collaborator

@modelonrobinandersson See latest commit for a few more things renamed for clarity/consistency.

Looks great! I think the renaming also helped a lot.

@PeterMeisrimelModelon PeterMeisrimelModelon merged commit 8ec1b43 into master Aug 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@modelonrobinandersson modelonrobinandersson modelonrobinandersson approved these changes

Assignees

@PeterMeisrimelModelon PeterMeisrimelModelon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeterMeisrimelModelon @modelonrobinandersson