refactor: Centralize error output #3902

arng40 · 2025-11-05T09:53:01Z

Remove code duplication found in GEOS_THROW, GEOS_ERROR, GEOS_WARNING and put into a static function in ErrorLogger.
Called while flushErrorMsg().
Move all Exceptions under GeosExceptions.hpp

…file

…necessary

… link between GEOS_THROW_CTX_IF and LVARRAY_THROW_IF_TEST( EXP, MSG, TYPE )

… in try/catch statements Problem: Retrieves everything that was thrown, so not just the message.

…/catch in main)": remove useless try/catch

…y spaces. The previous condition checked whether an argument was present and whether the option was immediately followed by a value like -test"value", which excluded valid cases like -test "value" et -test "value".

…t is expected.

…or-context

MelReyCG · 2025-12-17T10:36:48Z

src/coreComponents/common/logger/ErrorHandling.hpp

+    /// The signal received and formatted
+    std::string m_signal;


That little guy seems to never be initialized (addSignalToMsg()?)
Maybe you can just remove it and read the Attribute::Signal entry

src/coreComponents/common/logger/ErrorHandling.cpp

src/coreComponents/common/logger/ErrorHandling.hpp

src/coreComponents/common/logger/ErrorHandling.cpp

src/coreComponents/common/logger/ErrorHandling.hpp

MelReyCG · 2025-12-17T11:16:27Z

src/main/main.cpp

+  }
+  catch( std::exception const & e )
+  { // native exceptions management
+    ErrorLogger::ErrorMsg & errMsg = ErrorLogger::global().currentErrorMsg();


I think it lacks a lot of information here: at this point, IIRC, GEOS_THOW() has never been called, so the same information must be provided to the error message.

Appart from that, this part is a lot better!

…larity

MelReyCG · 2025-12-29T13:20:32Z

src/coreComponents/common/logger/ErrorHandling.cpp

+    oss << PREFIX<< "LOCATION: " << errMsg.m_file;
+    if( errMsg.m_line > 0 )
+    {
+      oss << " l." << errMsg.m_line;


You changed the format here in a way that prevent us, developpers, to ctrl+click the file location for quick access.

The LOCATION macro is:

#define LOCATION __FILE__ ":" STRINGIZE( __LINE__ )

Also, you put an if there but not in the YAML format.

MelReyCG · 2025-12-29T14:28:29Z

src/coreComponents/common/logger/ErrorHandling.hpp

+      :
+      m_type( msgType ),
+      m_msg( msgContent ),
+      m_ranksInfo( {rank} ),
+      m_file( msgFile ),
+      m_line( msgLine ),
+      m_commit( false ) {}


MelReyCG · 2025-12-29T14:30:59Z

src/coreComponents/common/logger/ErrorHandling.hpp

     * @return Reference to the current instance for method chaining.
     */
-    ErrorMsg & addToMsg( std::exception const & e, bool toEnd = false );
+    ErrorMsg & addToMsg( std::exception const & e, bool const toEnd = false );


Those const added to boolean values are implementation details, not interface matter, it should remain in the cpp.

same for MsgType and integer below

MelReyCG · 2025-12-29T14:35:40Z

src/coreComponents/common/logger/ErrorHandling.hpp

+    bool isCommited() const
+    { return m_commit; }


About this "commit" concept:

Why is it needed? What problem does it aim to solve? Is the "commited" state really needed?

ErrorMsg is a data structure with some helper methods for building it, I don't think it should have any state in it.

I see your new ErrorMsgBuilder class, with a lot of copied method, with some of them having silenciously no effect. Could the methods be better distributed?

I propose ErrorMsg being a POD, with no state, and ErrorMsgBuilder having all the building methods.
Also, I would not bind ErrorMsgBuilder to an ErrorLogger instance but to the ErrorMsg instance. This way you don't need this two ways dependancies anymore, which does not exist for good reasons (duplicating flush() & violating encapsulation).

MelReyCG · 2025-12-29T14:45:50Z

src/coreComponents/common/logger/ErrorHandling.hpp

-   *        potencially at various application layers
-   *        Use flushErrorMsg() when the message is fully constructed and you want it to be output
-   * @return Reference to the current instance for method chaining.
+   *        potencially at various application layers (Typically for exceptions)


"Typically for exceptions"
Are there other use-cases?

MelReyCG · 2025-12-29T15:57:55Z

src/coreComponents/common/logger/ErrorHandling.hpp

+  ErrorMsgBuilder beginLogger()
+  {
+    return ErrorMsgBuilder( *this );
+  }


I see that you tried to remove the "current error message" read-write accessor for encapsulation & clarity purpose, but I'm not sure the solution is better than the original: beginLogger() can be called multiple time in an exception stack scenario. I think buildCurrentErrorMsg() would be better named.
We could name flushErrorMsg() to flushCurrentErrorMsg() to emphasis the fact that the subject is the same.

MelReyCG · 2025-12-29T16:00:58Z

src/coreComponents/dataRepository/unitTests/testErrorHandling.cpp

    rank: 0
    message: >-
      Table input error.
-      Empty Group: Base Test Class (file.xml, l.23)
+      Empty Group
    contexts:
      - priority: 2
        inputFile: /path/to/file.xml


The main context information are lost (Base Test Class (file.xml, l.23)).

MelReyCG · 2025-12-29T16:02:34Z

src/coreComponents/common/logger/ErrorHandling.hpp

+
+private:
+    ///@copydoc ErrorLogger::m_errorContext
+    ErrorLogger & m_errorContext;


If you keep it:

Suggested change

ErrorLogger & m_errorContext;

ErrorLogger & m_errorLogger;

If you change it:

Suggested change

ErrorLogger & m_errorContext;

ErrorMsg & m_errorMsg;

MelReyCG · 2025-12-29T16:33:36Z

src/coreComponents/common/logger/ErrorHandling.hpp

+    ErrorContext( map< Attribute, std::string >  attributes, integer priority ):
+      m_dataDisplayString( "" ),
+      m_attributes( attributes ),
+      m_priority( priority ) {};


m_dataDisplayString is allowed as empty? Isn't that the source of your issue in the unit test?

MelReyCG · 2025-12-29T16:35:14Z

src/coreComponents/common/initializeEnvironment.cpp

+      ErrorLogger::global().beginLogger()
+        .addSignalToMsg( signal )
+        .setType( ErrorLogger::MsgType::Error )
+        .addRank( ::geos::logger::internal::g_rank )
+        .addCallStackInfo( stackHistory )
+        .addContextInfo(
+        ErrorContext{ { { ErrorContext::Attribute::Signal, std::to_string( signal ) } }, 1 },
+        ErrorContext{ { { ErrorContext::Attribute::DetectionLoc, string( "signal handler" ) } }, 0 } )
+        .flush();


Maybe a unit test for signals would be a good thing.

amandinehry added 30 commits April 25, 2025 17:05

creation of an object header that logs errors and warnings in a yaml …

c846586

…file

Set up ErrorHandling

f1713fa

traits errors commented (to treat)

4e5250f

Set up test

94b775c

Modification to properly format the yaml so that it can be parsed if …

1cb8aa3

…necessary

Test to add variadic arguments

9d5e16a

draft: retrieve data contexts info

2f7660c

Added additional information in the yaml - not functional missing the…

6036551

… link between GEOS_THROW_CTX_IF and LVARRAY_THROW_IF_TEST( EXP, MSG, TYPE )

First attempt at outputting exceptions

46b4904

Complete: code for outputting errors into yaml

e1aed93

Manage the text contained in the potential exception

bc953be

First attempt: Handling the text contained in the potential exception…

1a434bc

… in try/catch statements Problem: Retrieves everything that was thrown, so not just the message.

Complete: yaml output for PVT tables error

acf4d99

YAML error message well formatted

ee58884

Fix duplication error

32a0237

Add variadic parameters to GEOS_THROW_CTX_IF

589270f

Reorganization after operational tests

9b65581

Apply the output error functionality in yaml to all geos errors

b13f0a6

Latest update for exception handling (try/catch in main)

6de2c6a

Add priority to the error message into the yaml file

5830aee

Update of the commit named "latest update for exception handling (try…

2b27c41

…/catch in main)": remove useless try/catch

set priority

86f793e

errors cli modification: add --errors-output

8a8117e

errors cli modification: add --errors-output

2c0f5aa

Replace original macros

eb2f691

Correction: replace original macros

7b5f293

Reacting with the command line option -e / --errors-output

670ddfa

Add the rank on which the error is catch for error and warning outputs

d08a30f

Unit test to check if the contents of the yaml file match exactly wha…

5a6c1a0

…t is expected.

arng40 and others added 16 commits December 1, 2025 15:27

Merge remote-tracking branch 'origin/develop' into refactor/dudes/err…

b13149d

…or-context

missong doc for GeosException

e1b45f4

include error

976aab5

doxygen

42c53b9

update doxy

16c9e77

remove include, + some clean

30baf23

fix cuda compil

0aeddcf

move writeToAscii to cpp

ae60c25

doxygen

5a52a75

add log test

c48c52d

fix log wrong formatting

bbb72ff

update log formatting & output

855cf80

fix test

daec006

Uncrustify

ab9895b

Merge branch 'develop' into refactor/dudes/error-context

60a1f3a

Merge branch 'develop' into refactor/dudes/error-context

4e74b00

MelReyCG requested changes Dec 17, 2025

View reviewed changes

arng40 and others added 6 commits December 19, 2025 15:42

reorder function + add constructor to ErrorContext

08973a3

uncrustify & doxygen

cee0059

Merge branch 'develop' into refactor/dudes/error-context

2a639a7

Merge branch 'develop' into refactor/dudes/error-context

a2b31d1

improve writeToAscii

d4db78b

add context to std::exception

7df78ff

arng40 mentioned this pull request Dec 23, 2025

feat: Error reporting at the end of the simulatiuon #3929

Draft

arng40 added 4 commits December 24, 2025 15:33

fix encapsulation in ErrorLogger by adding BuilderPattern + improve c…

abc6d2a

…larity

uncrustify

ec7ddc1

remove redundant lmsg in case of multiple rank

4a5b4fe

missing const

35d8707

MelReyCG requested changes Dec 29, 2025

View reviewed changes

refactor: Centralize error output #3902

Are you sure you want to change the base?

refactor: Centralize error output #3902

Uh oh!

Conversation

arng40 commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arng40 commented Nov 5, 2025 •

edited

Loading