BerkeleyLab / caffeine

A parallel runtime library for Fortran compilers
https://berkeleylab.github.io/caffeine/
Other
37 stars 7 forks source link

Remove `stat` and `errmsg` args from `caf` collectives interfaces #86

Closed ktras closed 5 months ago

ktras commented 5 months ago

Closes #81.

ktras commented 5 months ago

@bonachea Thanks for the suggested changes. After making them, the function set_stat_errmsg_or_abort is never called. Is this a function we think will likely be useful in the future? Or should we delete the function? @rouson, @everythingfunctional, any thoughts?

everythingfunctional commented 5 months ago

I would have changed set_stat_errmsg_or_abort to call gasnett_fatalerror instead of hard-coding that multiple places.

bonachea commented 5 months ago

I would have changed set_stat_errmsg_or_abort to call gasnett_fatalerror instead of hard-coding that multiple places.

@everythingfunctional what you suggest is essentially the "before" code. The "after" code I suggested with a direct call will generate a more useful error message.

bonachea commented 5 months ago

@bonachea Thanks for the suggested changes. After making them, the function set_stat_errmsg_or_abort is never called. Is this a function we think will likely be useful in the future? Or should we delete the function? @rouson, @everythingfunctional, any thoughts?

@ktras I'm in favor of removing the dead code. I don't expect we'll be supporting any non-fatal errors anytime soon other than allocation failure, and that doesn't use this code path.

everythingfunctional commented 5 months ago

Ok, but then we should adjust the interface to set_stat_errmsg_or_abort to allow for passing the information/message we want, rather than getting rid of it. At some point we will want the logic it describes, and will be easier to make the change in one place instead of multiple.

That said, if you think

I don't expect we'll be supporting any non-fatal errors anytime soon other than allocation failure, and that doesn't use this code path.

I'm fine with just delaying that refactoring for now.