Feature/capps2/successreturnvalue

[agcapps]

Summary

• This PR is a new feature
• It does the following:
  • Refactors Group to return boolean success value from load(), save(), and loadExternalData()
  • Resolves Group::load() and save() should return a success value #101

Design review

On 13 November 2023, we (re)reviewed this PR. We decided that the extra information to the user was worthy of adding the feature.

[agcapps]

There are several ways we could add a boolean return value to load, save, and loadExternalData. This is one.

Alternately, we could

• make static bool axom::sidre::s_conduit_had_error into a member of DataStore
• make setConduitSLICMessageHandlers and setConduitDefaultMessageHandlers instance methods, not static

[kennyweiss]

This change in the PR would require a single global DataStore instance.

Is there a way to satisfy this issue without using global state?
E.g. registering a callback with conduit that modifies a local variable if there's an error? Or locally catching the conduit exception?

[agcapps]

This change in the PR would require a single global DataStore instance.

Not sure how it requires a single global DataStore instance.

Is there a way to satisfy this issue without using global state? E.g. registering a callback with conduit that modifies a local variable if there's an error? Or locally catching the conduit exception?

As I wrote, I would prefer storing the error flag as an instance variable on DataStore. The (possibly minor) obstacle is that DataStore::setConduitSLICMessageHandlers() and DataStore::setConduitDefaultMessageHandlers() are static. So currently, we could start out setting a per-DataStore flag on error, but a call to those two functions cannot touch anything that stores state in a DataStore.

[agcapps]

After a spirited discussion on November 27 2023, the consensus was that we (Axom developers) don't like the static global variable. Within load() and allies, we will temporarily save the error handler, set it to default, and do the Conduit operations within a try/catch block. Afterward, restore the error handler.

This idea will let us be more specific when reporting what happened, and allows each DataStore to hold its own state.

I'll rework the PR to reflect this idea and resubmit for review.

[kennyweiss]

This change in the PR would require a single global DataStore instance.

Not sure how it requires a single global DataStore instance.

What I meant was that if you had more than one DataStore instance, there would be no way to know which instance set the conduit_had_error variable. So I suppose I should have written that

This change in the PR would require likely be confusing if there was more than a single global DataStore instance.

[agcapps]

@nselliott , @kennyweiss , @cyrush , @rhornung67 , I'd appreciate your reviews. Thanks.

[nselliott]

Thanks Arlie. Approving with one comment on a small thing that should be fixed.

[kennyweiss]

Thanks @agcapps

Looks good overall.
My main question relates to resetting the error flags -- whose responsibility is it and when does it occur?

[kennyweiss]

As above, the code should return false if an invalid protocol was passed.
Please also check the other similar cases.
(Note SLIC_ERROR can be configured to not call exit).

[kennyweiss]

Would a std::list<std::string> (or similar) be better than appending strings on the fly?

A benefit would be that DataStore would not need to explicitly manage a separate bool for m_conduit_error_occurred -- it could instead be m_conduit_errors.empty().

[kennyweiss]

Are there any cases that test the EXPECT_FALSE path?
E.g. by setting slic::disableAbortOnError()

axom/src/axom/slic/interface/slic.hpp

Lines 133 to 140 in 65dfc3a

	/*!
	* \brief Disables aborts on error messages for the current active logger.
	*
	* \note this is equivalent to calling slic::setAbortOnError( false )
	* \post slic::isAbortOnErrorsEnabled() == false.
	* \pre slic::isInitialized() == true
	*/
	void disableAbortOnError();

[agcapps]

There are no cases that test EXPECT_FALSE, at this point. I should, indeed, try that.

[agcapps]

Added tests that use EXPECT_FALSE.

[kennyweiss]

I might be missing it, but where are the errors cleared?

Is that the user's responsibility? If so, is it documented and/or exercised in the sidre tests and examples?

[agcapps]

The user should clear errors as and when the user sees fit. I'm in the process of adding tests and documentation.

[agcapps]

Thanks @agcapps

Looks good overall. My main question relates to resetting the error flags -- whose responsibility is it and when does it occur?

The user has the responsibility to reset any error flags, or not, as needed. Most times, the user will probably detect the error and abort the simulation. More sophisticated error-handling is up to the user.