api: Add OIIO_NODISCARD_ERROR to ImageInput open and valid_file methods#5217
api: Add OIIO_NODISCARD_ERROR to ImageInput open and valid_file methods#5217PandorasPalette wants to merge 3 commits into
Conversation
|
Hi, there was a checklist when you filled out the PR submission, and one item was
I noticed you do not have that one checked, and I would really like to encourage you to actually do this before submitting a PR. Thanks! |
| /// it exits scope or is reset. The pointer will be empty if the | ||
| /// required writer was not able to be created. | ||
| static unique_ptr open (const std::string& filename, | ||
| OIIO_NODISCARD_ERROR static unique_ptr open (const std::string& filename, |
There was a problem hiding this comment.
This should be OIIO_NODISCARD because it's an important result that is the very reason for calling open, and we can assume that the call is either wrong or superfluous if the return value is discarded.
We use OIIO_NODISCARD_ERROR only for error return codes, where the code is conceivably correct without capturing the return value, as long as no errors actually occur. (Though it is careless to not checking for errors and handle them graefully).
Despite this, all tests pass! But we will need you to fix the DCO and CLA, in addition to the other comment I left on the code. |
lgritz
left a comment
lgritz
left a comment
There was a problem hiding this comment.
Hi, this still needs fixes as described, and also it needs a DCO signoff on every commit for us to be able to merge it.
|
|
||
| /// Check valid file using a UTF-16 encoded wstring filename. | ||
| bool valid_file (const std::wstring& filename) const { | ||
| OIIO_NODISCARD_ERROR bool valid_file (const std::wstring& filename) const { |
There was a problem hiding this comment.
This, too, should be OIIO_NODISCARD because the result is not an error status, it's just a bool value that tells you whether or not the file is valid.
2 participants
Description
Tests
No new testsuite case was added.
Checklist:
and if I used AI coding assistants, I have an
Assisted-by: TOOL / MODELline in the pull request description above.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.