feat: Missed std::map

[castelletto1]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.82%. Comparing base (efe09c2) to head (9fe6b6a).

Files with missing lines	Patch %	Lines
src/coreComponents/schema/schemaUtilities.cpp	0.00%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3877      +/-   ##
===========================================
- Coverage    58.82%   58.82%   -0.01%     
===========================================
  Files         1321     1321              
  Lines       108923   108924       +1     
===========================================
  Hits         64077    64077              
- Misses       44846    44847       +1     

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

[MelReyCG]

Suggested change

	isTargetSetCreated[setNames[i]] = 0;
	isTargetSetCreated.get_inserted( setNames[i] ) = 0;

@castelletto1 @arng40 ?

[MelReyCG]

My question is, how the isTargetSetCreated map gets populated if only the operator[] is used?

[arng40]

Indeed we must set the getInserted() method

[MelReyCG]

And the question it brings: as geos::stdMap::operator[] is only for getting map elements and should throw an error if bound checking is enabled, how can that pass the tests?
if there is a deeper issue, it will need another PR @arng40

[arng40]

It's fine, for the moment isTargetSetCreated is a map and not a stdMap that's why the CI is running and ok

[rrsettgast]

The goal was to toggle geos::map between geos::stdMap and std::map. So we would always use geos::map in the code and get either behavior depending on the USE_STD_CONTAINER_BOUNDS_CHECKING/GEOS_USE_BOUNDS_CHECK is on. It seems we have a couple of inconsistencies here? Likewise for geos::unordered_map.

[MelReyCG]

There is no "std bounds checking" on std map containers, std::*map::operator[] creates elements, this forces us to have a wrapper over std map classes. At this point I think we can just merge geos::stdMap and geos::map, and it will be configurable with GEOS_USE_BOUNDS_CHECK

• when true, geos maps will throw proper exception/error when trying to access non-existing element through operator[], as planned in the draft,
• when false, geos maps will just silently create new elements for non-existing elements.

[rrsettgast]

@MelReyCG Yes, I am aware of that. I am pointing out that the std container wrappers that we are creating should not be directly used. If GEOS_USE_BOUNDS_CHECK then we use them. If not, then we use the actual std containers. This should be done via an alias that is protected via the preprocessor guards on GEOS_USE_BOUNDS_CHECK

[MelReyCG]

About consistency, should we have a geos::vector then? (we now use geos::stdVector everywhere)

[MelReyCG]

I think there are a few redondancies for now in this GEOS_USE_BOUNDS_CHECK behaviour (around the types/operators).

[MelReyCG]

@wrtobin Here you have a typical instance of the current issue of GEOS error handling. The Ubuntu 22.04, clang 15.0.7, open-mpi 4.1.2 crashed, and here is all the log we get:

Functions: adding SymbolicFunction SymbolicFunction
Functions: adding CompositeFunction CompositeFunction
Functions: adding MultivariableTableFunction MultivariableTableFunction
Functions: adding TableFunction TableFunction
  SymbolicFunction
  CompositeFunction
  MultivariableTableFunction
  TableFunction
Generating XML Schema...
  Basic datatypes
--------------------------------------------------------------------------
MPI_ABORT was invoked on rank 0 in communicator MPI_COMM_WORLD
with errorcode 1.

NOTE: invoking MPI_ABORT causes Open MPI to kill all MPI processes.
You may or may not see output from other processes, depending on
exactly when Open MPI kills them.
--------------------------------------------------------------------------

No reason, no stacktrace, and no way to add info.
This is one of the motivation of the #3828 and #3856. On P4 and on other machines, we have a log with no trace of error, and the stderr has the MPI_ABORT errors, which is not informative.

[rrsettgast]

The goal was to toggle geos::map between geos::stdMap and std::map. So we would always use geos::map in the code and get either behavior depending on the USE_STD_CONTAINER_BOUNDS_CHECKING/GEOS_USE_BOUNDS_CHECK is on. It seems we have a couple of inconsistencies here? Likewise for geos::unordered_map.