Never return nullptr silently

[sjahr]

Description

• Create some warning/ error message before returning a nullptr for better debugability
• Some minor documentation improvement

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[rhaschke]

Issueing an error message before returning a nullptr is not always desired: It might be perfectly possible that the caller correctly handles the nullptr result and issues an error itself or handles the error condition gracefully in some other fashion.
In that case, the error message would be spurious.
At most, you should issue a debug message.

[marioprats]

I agree with Robert.
The functions returning nullptr are indicative of an error, which the caller should handle.
A better solution would be to introduce a good error propagation mechanism, like tl::expected or absl::Status. Then we could return an error with a string, and let the caller decide what to do.
In the meanwhile I think a log message makes sense, but I would make it a debug message.

[rhaschke]

The last commit (57a85c8) should be handled in a separate PR. This is a fundamental change of the API and logic, which should be well motivated. I don't yet see the "ugliness" of the current API.
You should better address the reviews and change error messages to debug messages.

[sjahr]

The last commit (57a85c8) should be handled in a separate PR. This is a fundamental change of the API and logic, which should be well motivated. I don't yet see the "ugliness" of the current API. You should better address the reviews and change error messages to debug messages.

You're right, that derailed to initial intention of the PR too much, sorry. I reverted it and updated the log level to debug in most cases. Let me know what you think.

I don't yet see the "ugliness" of the current API.

Maybe isn't the right term but rather outdated. I think we shouldn't pass raw boolean pointers anywhere in modern C++ but as you pointed out correctly, this PR is not the place to try changing that.

[rhaschke]

See comments below.

[rhaschke]

Sorry, I missed some closing parentheses.

[rhaschke]

Fix parentheses. Still unchecked. I just did that in the Web GUI.

[henningkayser]

@sjahr unfortunately this needs a new iteration because I merged #2985

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[mergify]

This pull request is in conflict. Could you fix it @sjahr?

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.