riscv-software-src / riscof

BSD 3-Clause "New" or "Revised" License
61 stars 39 forks source link

Raises do not raise #64

Open marcfedorow opened 1 year ago

marcfedorow commented 1 year ago

https://github.com/riscv-software-src/riscof/blob/b86c5079e4f46ca53258cf2cf438b5308f534cb7/riscof/Templates/setup/model/riscof_model.py#L183

This place (and some more) calls raise that calls exit. Exit exits before raise raises. This is very strange construction to me, especially raise on success. What are intentions of it?

pawks commented 1 year ago

This place (and some more) calls raise that calls exit. Exit exits before raise raises.

The "raise" doesn't really call a "SystemExit" in the typical sense you are referring to here. Here the SystemExit(0) is actually creating an object of type "SystemExit" with 0 being the exit code. raise is the keyword used to raise exceptions in python explicitly. It offers a graceful way of exiting the program while offering the caller function the alternative of "catching" the exception and digressing down a different code path.

Raise on success shouldn't be happening probably. The raise here will almost always kill the execution unless its caught and handled in the framework(which isn't done). I suppose it was an artifact of a copy at this particular line in question.

marcfedorow commented 1 year ago

Why not just sys.exit(0) or exit(0)? Raise on success is counterintuitive. In lines like this https://github.com/riscv-software-src/riscof/blob/b86c5079e4f46ca53258cf2cf438b5308f534cb7/riscof/Templates/setup/model/riscof_model.py#L33 it could be more convenient to use something like

if config is None:
  raise SomeException('Please enter input file paths in configuration.')

than print an error to stdout and then just exit with a non-zero exit code.

pawks commented 1 year ago

Raise on success is counterintuitive.

The raise on success should definitely be removed as the plugins should not be terminating the execution on success.

it could be more convenient to use something like

if config is None:
 raise SomeException('Please enter input file paths in configuration.')

than print an error to stdout and then just exit with a non-zero exit code.

Currently riscof does not have any custom exceptions defined for the plugins to use. It can definitely be added to allow for graceful termination.