FGInitialCondition::Load throws instead of returning false

[AirshipSim]

Following discussion #1407
Hint: Hide white space differences when checking Files changes

The function FGInitialCondition::Load was obviously patched up along the years. Upon error, it could

• throw a LogException (unhandled)
• throw XMLLogException (unhandled)
• return false

It is now unified to return false in all cases. Functions inside Load( ) which could throw are now guarded by a try / catch.

Other corrected minor issue: the reading of engine configuration was done directly in FGInitialCondition::Load( ), instead of being done in the functions Load_v1 or Load_v2 like any other tag.

Current usage in the codebase

In the codebase, Load( ) was called in two different places:

jsbsim/src/input_output/FGScript.cpp

Lines 195 to 200 in 98c0333

	auto IC = FDMExec->GetIC();
	if ( ! IC->Load( initialize )) {
	FGLogging log(LogLevel::ERROR);
	log << "Initialization unsuccessful\n";
	return false;
	}

jsbsim/src/JSBSim.cpp

Lines 477 to 482 in 98c0333

	auto IC = FDMExec->GetIC();
	if ( ! IC->Load(ResetName)) {
	delete FDMExec;
	cerr << "Initialization unsuccessful" << endl;
	exit(-1);
	}

In both cases, the return value ( true or false ) was correctly checked, nothing to do there.

Testing

Testing in command line, the errors are caught correctly and JSBSim terminates with the proper error message, instead of terminate suddenly with FATAL ERROR and less log:

Could not open file: Path "aircraft/c172r/reset0.xml"
Init file: Path "aircraft/c172r/reset0" could not be loaded.
Initialization unsuccessful
Script file Path "scripts/c1721.xml" was not successfully loaded

D:\DEV\jsbsim>

In file aircraft/c172r/reset00.xml: line 16
XML parse error: mismatched tag
Error parsing Path "aircraft/c172r/reset00"
In file aircraft/c172r/reset00.xml: line 16
XML parse error: mismatched tagInitialization unsuccessful
Script file Path "scripts/c1721.xml" was not successfully loaded

D:\DEV\jsbsim\Debug\JSBSim.exe (process 2552) exited with code -1 (0xffffffff).

[codecov]

Codecov Report

❌ Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.21%. Comparing base (077e835) to head (a57360d).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
src/initialization/FGInitialCondition.cpp	0.00%	34 Missing ⚠️
src/input_output/FGScript.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1408      +/-   ##
==========================================
- Coverage   25.22%   25.21%   -0.02%     
==========================================
  Files         169      169              
  Lines       18633    18641       +8     
==========================================
  Hits         4701     4701              
- Misses      13932    13940       +8     

☔ 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.

[bcoconni]

Functions inside Load( ) which could throw are now guarded by a try / catch.

That's a hard no-go as far as I am concerned: #1407 (comment)

[AirshipSim]

Functions inside Load( ) which could throw are now guarded by a try / catch.

That's a hard no-go as far as I am concerned: #1407 (comment)

@bcoconni You are the person telling me to open a PR at the end of the discussion... Logic has left the chat.

[bcoconni]

@bcoconni You are the person telling me to open a PR at the end of the discussion... Logic has left the chat.

First, I have indeed told you to open the PR but never implied that we would accept your change upfront.

Second, as I explained earlier (see #1407 (comment)):

• I am voting against the "hide everything under the carpet" option:

try {
	if (!Exec->LoadScript(SGPath{ ResetPath}, dt, SGPath{ InitPath }))
	{
		return false; 
	}
}
catch (const JSBSim::LogException&) {
	return false;
}

• The problem with try/catch is that once they are there, it's very difficult to get rid of them. You can never be sure that they are not intercepting an exception thrown by a convoluted call stack. I'd rather keep the code as it is currently.

So I find that I am pretty consistent with myself.

Third, my previous comment may have sounded firm, but it focused solely on the technical aspects. There was no personal remark or judgment. I expect discussions to challenge facts and design choices, not individuals.

So I stand by my position. And anyone is free to challenge my opinions even when they are strong.

As far as I am concerned, the discussion remains open to technical arguments.

[seanmcleod70]

@AirshipSim @bcoconni did make his views on swallowing exceptions quite clear in the discussion. You're free to still add a try/catch to your PR, but you can't be surprised that @bcoconni will re-iterate the point he made in the discussion.