Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cob_call_with_exception_check (test 771) #129

Conversation

ddeclerck
Copy link
Contributor

No description provided.

@ddeclerck ddeclerck linked an issue Dec 13, 2023 that may be closed by this pull request
@ddeclerck ddeclerck changed the title Fix cob_call_with_exception_check Fix cob_call_with_exception_check (test 771) Dec 13, 2023
@ddeclerck ddeclerck requested a review from GitMensch December 13, 2023 09:13
@ddeclerck ddeclerck force-pushed the fix_call_with_exception_check branch from 02f7de6 to 52f4cb5 Compare December 13, 2023 09:19
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0d64ad) 65.74% compared to head (52f4cb5) 65.74%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                Coverage Diff                 @@
##           gcos4gnucobol-3.x     #129   +/-   ##
==================================================
  Coverage              65.74%   65.74%           
==================================================
  Files                     32       32           
  Lines                  59092    59096    +4     
  Branches               15575    15576    +1     
==================================================
+ Hits                   38849    38854    +5     
  Misses                 14262    14262           
+ Partials                5981     5980    -1     

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

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to skip the complete termination?
I think we should do the screenio cleanup - but only if that was done after the call to cob_call_with_exception_check().
We very likely need to call the exit handlers, as those can be COBOL programs which need the state "at exit", not any previous state we longjmp to. But then those should not be executed after the longjmp.

We possibly should add some additional testcases that combine cob_call_with_exception_check() with the scenario "exception and exit handlers setup".

@ddeclerck ddeclerck force-pushed the fix_call_with_exception_check branch from 52f4cb5 to 2db2b90 Compare December 14, 2023 10:06
@ddeclerck
Copy link
Contributor Author

ddeclerck commented Dec 14, 2023

Is there a reason to skip the complete termination?

Well, in #125, you suggest :

I'd prefer the second - and document that modules will only be unloaded with this function if after the call a manual call to cob_tidy() is done.

Since cob_tidy calls call_exit_handlers_and_terminate, it seemed natural to just skip the whole termination.

Anyways, I pushed an improved version, where only the last bits of termination are delayed, ie only the following calls :

	cob_exit_call ();
	cob_exit_common ();

Cleanup is automatically performed after the longjump, so no need to call cob_tidy.

@ddeclerck ddeclerck force-pushed the fix_call_with_exception_check branch from 2db2b90 to ccfebd6 Compare December 14, 2023 10:10
@GitMensch GitMensch self-requested a review December 14, 2023 14:40
Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very clean. Please add a changelog entry, then this is good for svn (sight, I won't finish the parts to make svn ready for more commits until moid/end next week).

@ddeclerck ddeclerck force-pushed the fix_call_with_exception_check branch from ccfebd6 to 44bc538 Compare December 14, 2023 15:04
@ddeclerck ddeclerck closed this Jan 22, 2024
@ddeclerck ddeclerck deleted the fix_call_with_exception_check branch July 15, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test 772 fails on Windows
3 participants