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 discovery of document version number #845

Merged
merged 2 commits into from
Mar 4, 2023
Merged

Fix discovery of document version number #845

merged 2 commits into from
Mar 4, 2023

Conversation

heshpdx
Copy link
Contributor

@heshpdx heshpdx commented Feb 27, 2023

The code at initialization/FGInitialCondition.cpp:1036 is problematic for some newer compilers. I think version was coming back as inf or maybe -inf (I am not sure, it was not on my machine). Reports came in like this:

   In file aircraft/B747/reset00.xml: line 2

   Only initialization file formats 1 and 2 are currently supported
   FATAL ERROR: JSBSim terminated with an unknown exception.

I investigated and tried to figure the intent of the code. It feels like we should only check the version number if it was read in from the file, and otherwise just do a Load_v1. I moved the block around and eliminated the if (version == HUGE_VAL) line which looked fishy (exact FP comparisons are usually not portable).

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #845 (22375ed) into master (ebb770e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #845   +/-   ##
=======================================
  Coverage   22.43%   22.43%           
=======================================
  Files         167      167           
  Lines       19610    19608    -2     
=======================================
  Hits         4400     4400           
+ Misses      15210    15208    -2     
Impacted Files Coverage Δ
src/initialization/FGInitialCondition.cpp 40.82% <0.00%> (+0.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@bcoconni bcoconni left a comment

Choose a reason for hiding this comment

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

Overall looks good to me but I think the version variable declaration should now be located in the if (document->HasAttribute("version")) { block (see my review comments).

src/initialization/FGInitialCondition.cpp Outdated Show resolved Hide resolved
src/initialization/FGInitialCondition.cpp Outdated Show resolved Hide resolved
Move local variable to inner scope.

Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Copy link
Member

@bcoconni bcoconni left a comment

Choose a reason for hiding this comment

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

Fine ! Thank you 👍

@bcoconni bcoconni merged commit b2172a4 into JSBSim-Team:master Mar 4, 2023
bcoconni pushed a commit to bcoconni/jsbsim that referenced this pull request Apr 8, 2023
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