DRAFT: Fix warnings from new compilers #1291

heshpdx · 2025-05-27T22:36:16Z

Latest compilers uncovered some warnings, mostly regarding mismatched virtual function signatures.

type of 'ptm' does not match original declaration [-Wlto-type-mismatch] 
'virtual bool JSBSim::FGModel::Load(JSBSim::Element*)' was hidden [-Woverloaded-virtual=]
'virtual double JSBSim::FGEngine::GetPowerAvailable()' was hidden [-Woverloaded-virtual=]
'virtual double JSBSim::FGThruster::GetPowerRequired()' was hidden [-Woverloaded-virtual=]

#834

type of 'ptm' does not match original declaration [-Wlto-type-mismatch] 'virtual bool JSBSim::FGModel::Load(JSBSim::Element*)' was hidden [-Woverloaded-virtual=] 'virtual double JSBSim::FGEngine::GetPowerAvailable()' was hidden [-Woverloaded-virtual=] 'virtual double JSBSim::FGThruster::GetPowerRequired()' was hidden [-Woverloaded-virtual=]

codecov · 2025-05-27T22:45:54Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 24.76%. Comparing base (024046d) to head (ec3024e).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/models/FGModel.h	0.00%	1 Missing ⚠️
src/models/propulsion/FGEngine.h	0.00%	1 Missing ⚠️
src/models/propulsion/FGThruster.h	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1291      +/-   ##
==========================================
- Coverage   24.76%   24.76%   -0.01%     
==========================================
  Files         169      169              
  Lines       19598    19600       +2     
==========================================
  Hits         4854     4854              
- Misses      14744    14746       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

heshpdx · 2025-05-28T13:54:42Z

This isn't the full fix. WIP...

bcoconni

Thanks for submitting this PR @heshpdx.

I have made a couple of suggestions about how the code from JSBSim should be modified to silence the warnings. Please review them and let us know about your feedback.

bcoconni · 2025-05-29T08:40:12Z

src/models/FGModel.h

@@ -97,6 +97,7 @@ class JSBSIM_API FGModel : public FGModelFunctions
  virtual SGPath FindFullPathName(const SGPath& path) const;
  const std::string& GetName(void) const { return Name; }
  virtual bool Load(Element* el) { return true; }
+  virtual bool Load(Element* el, const SGPath& dir) { return true; }


Suggested change

virtual bool Load(Element* el, const SGPath& dir) { return true; }

Don't move a method up (i.e. from a daughter class to a mother class) just for the sake of muting compiler warnings ! See my comment on the file src/models/FGOutput.h below for an hopefully better fix.

bcoconni · 2025-05-29T08:44:34Z

src/models/FGOutput.h

@@ -195,7 +195,7 @@ class JSBSIM_API FGOutput : public FGModel
      @param el XMLElement that is pointing to the output directives
      @param dir optional directory path to load included files from
      @result true if the execution succeeded. */
-  bool Load(Element* el, const SGPath& dir = SGPath());
+  bool Load(Element* el, const SGPath& dir = SGPath()) override;


Suggested change

bool Load(Element* el, const SGPath& dir = SGPath()) override;

using FGModel::Load;

bool Load(Element* el, const SGPath& dir = SGPath());

The instruction using FGModel::Load is telling the compiler to "un-hide" the method from the inherited class FGModel. This is normally silencing compilers warnings about hidden methods.

Why C++ is hiding by default inherited methods with dissimilar signatures is beyond my comprehension but that's how this language works.

Why C++ is hiding by default inherited methods with dissimilar signatures is beyond my comprehension

https://www.stroustrup.com/bs_faq2.html#overloadderived

Hmm, not so sure about the using FGModel::Load; option, given the default argument for dir it means the compiler is going to say some of the calls are ambiguous.

bool FGModel::Load(Element* el); bool FGOutput::Load(Element* el, const SGPath& dir = SGPath()); fgOutput = new FGOutput(); fgOutput->Load(some_element); // Ambiguous, which Load() to call?

Why C++ is hiding by default inherited methods with dissimilar signatures is beyond my comprehension

https://www.stroustrup.com/bs_faq2.html#overloadderived

Well, it seems to be proving my point: "The compiler [...] never bothers with the (enclosing) scope [...]" says Stroustrup. But he doesn't explain why they have made that choice in the first place. Here it is just exposed as a fact of life, an obvious statement. The question he is not answering is "Why does the compiler not bother with the enclosing scope ?".

Also of interest is the choice of the word hidden used by compilers to qualify this situation. I'm not a native speaker but I see some quite different meanings between "not bothering with" (Stroustrup) and "hiding" (compilers). Is the C++ specification actually coded in the compilers by hiding methods from inherited classes ?

Also note that:

Our code that is triggering the warning is not trying to use the hidden method. It is using a method with the same name but with a different signature. So there is no error involved in our code.

I guess the compiler is concerned that we might have wrongly created a new method instead of using the method inherited from FGModel. Once the statement using FGModel::Load; is added the compiler is assuming the new method has been created on purpose but it has actually no guarantee whatsoever that the intention of the programmer is correct either way.

This way we are explicitly hiding the method ourselves and tell "To hell the compiler, we know what we are doing!".

Well, the F word came to my mind first but we are gentlemen, aren't we ?

Hmm, you may want to break out some French swear words...

So, making the using FGModel::Load private doesn't help in the one instance, i.e. from the call to Load from within the FGOutput class itself in FGOutput::SetDirectivesFile().

Hmm, and I just tested it now in MSVC, i.e. making using FGModel::Load private, and I still get 2 compiler errors, the one is expected in terms FGOutput::SetDirectivesFile() but it's still giving an error for the call in FGFDMExec. Possibly a bug in the MSVC compiler?

Hmm, some basic tests with MSVC. Bit confused by Case 2, almost as if the compiler does an ambiguity check before it checks access control and outputs an ambiguity error as opposed to excluding the ambiguous method first based on access control and then checking the resulting list of possible methods for ambiguity?

Case 1
Warning function is hidden.

class Base { public: virtual void show(int i) {} }; class Derived : public Base { public: void show(int i, int j=5) {} private: }; int main() { Derived* d = new Derived(); d->show(4); }

Case 2
Error ambiguous call to overloaded function.

class Base { public: virtual void show(int i) {} }; class Derived : public Base { public: void show(int i, int j=5) {} private: using Base::show; }; int main() { Derived* d = new Derived(); d->show(4); }

Case 3
Error 'Derived::show': cannot access private member declared in class 'Derived'

class Base { public: virtual void show(int i) {} }; class Derived : public Base { public: void show(int i, int j) {} private: using Base::show; }; int main() { Derived* d = new Derived(); d->show(4); }

So how about this as a potential solution?

class FGOutput : public FGModel { bool Load(Element* el) { return Load(el, SGPath()); } bool Load(Element* el, const SGPath& path); };

So basically provide an overridden implementation of FGModel::Load which simply calls the existing FGOutput::Load passing it an SGPath, with the only other change being the removal of the default argument.

No other code changes required to any of the other code making use of FGOutput.

That's very simple, I like it.

bcoconni · 2025-05-29T09:07:03Z

src/models/atmosphere/MSIS/nrlmsise-00.c

@@ -61,7 +61,7 @@ extern double pma[10][100];
 extern double sam[100];

 /* LOWER7 */
-extern double ptm[10];
+extern double ptm[50];


This one is a bit trickier since this is imported code from the MSIS library. To address this issue, I have grep'ed the code.

First, counting the number of occurrences of ptm:

> cd src/models/atmosphere/MSIS > grep 'ptm' *.c *.h | wc -l 40

so there are 40 occurrences of the name ptm in the code.

Now looking for occurrences of ptm that are of the form ptm[1], or ptm[ 15 ], etc.

> grep -E 'ptm\s*\[\s*[0-9]+\s*\]' *.c *.h | wc -l 40

Same number than above so there is no pointer arithmetic such as ptm + i or ptm[i] (where i is a variable) nor any pointer de-referencing such as *ptm.

Then looking for occurrences of ptm[X] where X is a single figure i.e. 0<=X<=9:

> grep -E 'ptm\s*\[\s*[0-9]\s*\]' *.c *.h | wc -l 38

So there remains 2 occurrences using more than 1 figure (such as ptm[31] or ptm [142], etc.).

Now displaying the occurrences of ptm[] with exactly 2 figures:

> grep -nE 'ptm\s*\[\s*[0-9][0-9]\s*\]' *.c *.h nrlmsise-00.c:64:extern double ptm[10]; nrlmsise-00_data.c:388:double ptm[50] = {

Two declarations! These are the remaining occurrences of ptm, the ones that are using more than 1 figure.

Now we can tell that the code is always using the ptm array with explicit indices 0,1,...,9 except for the declarations which do not match in their declared size of ptm.

The first occurrence in nrlmsise-00.c, line 64 is a declaration of ptm:

jsbsim/src/models/atmosphere/MSIS/nrlmsise-00.c

Line 64 in 6d96ff0

extern double ptm[10];

The second occurrence in nrlmsise-00_data.c, line 388 is the actual definition of ptm:

jsbsim/src/models/atmosphere/MSIS/nrlmsise-00_data.c

Lines 388 to 391 in 6d96ff0

double ptm[50] = {

1.04130E+03, 3.86000E+02, 1.95000E+02, 1.66728E+01, 2.13000E+02,

1.20000E+02, 2.40000E+02, 1.87000E+02,-2.00000E+00, 0.00000E+00

};

Tada 🎉 the definition is allocating 50 slots but is actually using only 10 of them since all the references in nrlmsise-00.c are of the form ptm[0], ptm[1], ..., ptm[9].

So the actual size of ptm should be 10 and I'm suggesting that you change 50 to 10 in nrlmsise-00_data.c instead of what you have suggested above.

My check was a bit more manual last night without using regular expressions 😉 Basically searched for ptm and manually checked each instance for pointer arithmetic and confirmed all the indices were between 0 and 9.

No problem! At least we are reaching the same conclusion 😉

bcoconni · 2025-05-29T09:18:28Z

This isn't the full fix. WIP...

No problem! Please take into account the comments I have made above (especially about using C++'s using rather than moving methods up in the class hierarchy) before submitting an update

seanmcleod70 · 2025-05-28T11:53:58Z

src/models/atmosphere/MSIS/nrlmsise-00.c

@@ -61,7 +61,7 @@ extern double pma[10][100];
 extern double sam[100];

 /* LOWER7 */
-extern double ptm[10];
+extern double ptm[50];


Hmm, I think the correct fix for this warning is to instead change ptm[50] to ptm[10] over here:

jsbsim/src/models/atmosphere/MSIS/nrlmsise-00_data.c

Lines 388 to 391 in 6d96ff0

double ptm[50] = {

1.04130E+03, 3.86000E+02, 1.95000E+02, 1.66728E+01, 2.13000E+02,

1.20000E+02, 2.40000E+02, 1.87000E+02,-2.00000E+00, 0.00000E+00

};

All the other references to ptm[x] are for constants between 0 and 9.

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+ptm%5B&type=code

seanmcleod70 · 2025-05-28T11:58:26Z

src/models/propulsion/FGEngine.h

@@ -180,7 +180,7 @@ class FGEngine : public FGModelFunctions

  virtual double CalcOxidizerNeed(void) {return 0.0;}

-  virtual double GetPowerAvailable(void) {return 0.0;};
+  virtual double GetPowerAvailable(void) const {return 0.0;};


There are 7 instances and it looks like there are a number of other cases where const hasn't been used?

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+GetPowerAvailable&type=code

seanmcleod70 · 2025-05-28T11:59:43Z

src/models/propulsion/FGThruster.h

@@ -92,7 +92,7 @@ class FGThruster : public FGForce {
  void SetName(std::string name) {Name = name;}
  virtual void SetRPM(double rpm) {};
  virtual void SetEngineRPM(double rpm) {};
-  virtual double GetPowerRequired(void) {return 0.0;}
+  virtual double GetPowerRequired(void) const {return 0.0;}


Also looks like there are multiple cases where const is missing?

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+GetPowerRequired&type=code

seanmcleod70 · 2025-05-29T09:48:15Z

src/models/atmosphere/MSIS/nrlmsise-00.c

@@ -61,7 +61,7 @@ extern double pma[10][100];
 extern double sam[100];

 /* LOWER7 */
-extern double ptm[10];
+extern double ptm[50];


My check was a bit more manual last night without using regular expressions 😉 Basically searched for ptm and manually checked each instance for pointer arithmetic and confirmed all the indices were between 0 and 9.

heshpdx changed the title ~~Fix warnings from new compilers~~ DRAFT: Fix warnings from new compilers May 28, 2025

bcoconni requested changes May 29, 2025

View reviewed changes

seanmcleod70 reviewed May 29, 2025

View reviewed changes

	bool Load(Element* el, const SGPath& dir = SGPath()) override;
	using FGModel::Load;
	bool Load(Element* el, const SGPath& dir = SGPath());

	double ptm[50] = {
	1.04130E+03, 3.86000E+02, 1.95000E+02, 1.66728E+01, 2.13000E+02,
	1.20000E+02, 2.40000E+02, 1.87000E+02,-2.00000E+00, 0.00000E+00
	};

DRAFT: Fix warnings from new compilers #1291

Are you sure you want to change the base?

DRAFT: Fix warnings from new compilers #1291

Conversation

heshpdx commented May 27, 2025

Uh oh!

codecov bot commented May 27, 2025

Codecov Report

Uh oh!

heshpdx commented May 28, 2025

Uh oh!

bcoconni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoconni May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoconni May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoconni commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bcoconni May 29, 2025 •

edited

Loading

bcoconni May 29, 2025 •

edited

Loading