Pull request: https://github.com/aibasel/downward/pull/223

Merged.

Looks good to merge to me now. Thanks for taking care of this, Jendrik!

Looks good! I noticed that our implementation of exit_with has the same problem as described below of mixing buffered with unbuffered output. I left a comment with the pull request, otherwise no further comments.

I followed your suggestion with the custom exception and made a pull request.

I think the change you describe isn't new code from Scorpion; it looks like the code we had in the main Fast Downward repository until recently (issue984). Looks like issue984 included a change that is questionable.

However, I also think using the reentrant code here is a problem because it doesn't play well with using iostreams, so would break as soon we replaced "endl" in the line above with "\n", which I think is too fragile. There is no need to use reentrant code here; I suggest we add a "normal" version of report_exit_code.

For what it's worth, exit calls destructors of non-automatic objects, so another way to make sure all destructors are called would be to wrap the lines above in "{" "}". But that's perhaps too subtle.

But if we're touching this call to exit_with, shouldn't we fix all calls to exit_with as well, by changing the implementation of exit_with rather than avoiding calling it in this one place? I think a standard strategy would be to raise an exception and catch it in main.

After finding a plan, Fast Downward calls exit_with() which in turn calls exit(). The exit() function performs some cleanup (https://en.cppreference.com/w/c/program/exit), but doesn't call destructors (https://www.hackerearth.com/practice/notes/return-0-vs-exit0-in-main/). This might be unexpected for users who'd like to report some statistics when objects are destroyed. Also, this behaviour leads to Valgrind reporting memory leaks:

./build.py && valgrind --leak-check=full builds/release/bin/downward --search "astar(blind())" --internal-plan-file sas_plan < output.sas
[...]
Solution found.
Peak memory: 171880 KB
==338825== 
==338825== HEAP SUMMARY:
==338825==     in use at exit: 43,075 bytes in 223 blocks
==338825==   total heap usage: 31,916 allocs, 31,693 frees, 1,712,735 bytes allocated
==338825== 
==338825== LEAK SUMMARY:
==338825==    definitely lost: 0 bytes in 0 blocks
==338825==    indirectly lost: 0 bytes in 0 blocks
==338825==      possibly lost: 0 bytes in 0 blocks
==338825==    still reachable: 43,075 bytes in 223 blocks
==338825==         suppressed: 0 bytes in 0 blocks
==338825== Reachable blocks (those to which a pointer was found) are not shown.
==338825== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==338825== 
==338825== For lists of detected and suppressed errors, rerun with: -s
==338825== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

In Scorpion, I solved this problem by using the following code at the end of main() in planner.cc:

    // exit_with() doesn't call destructors.
    report_exit_code_reentrant(exitcode);
    return static_cast<int>(exitcode);