Skip to content

api: Add OIIO_NODISCARD_ERROR to ImageInput open and valid_file methods#5217

Open
PandorasPalette wants to merge 3 commits into
AcademySoftwareFoundation:mainfrom
PandorasPalette:main
Open

api: Add OIIO_NODISCARD_ERROR to ImageInput open and valid_file methods#5217
PandorasPalette wants to merge 3 commits into
AcademySoftwareFoundation:mainfrom
PandorasPalette:main

Conversation

@PandorasPalette
Copy link
Copy Markdown

Description

Tests

No new testsuite case was added.

Checklist:

  • I have read the guidelines on contributions and code review procedures.
  • I have read the Policy on AI Coding Assistants
    and if I used AI coding assistants, I have an Assisted-by: TOOL / MODEL
    line in the pull request description above.
  • I have updated the documentation if my PR adds features or changes
    behavior.
  • I am sure that this PR's changes are tested in the testsuite.
  • I have run and passed the testsuite in CI before submitting the
    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.)
  • My code follows the prevailing code style of this project and I
    fixed any problems reported by the clang-format CI test.
  • If I added or modified a public C++ API call, I have also amended the
    corresponding Python bindings. If altering ImageBufAlgo functions, I also
    exposed the new functionality as oiiotool options.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 28, 2026

CLA Signed
The committers listed above are authorized under a signed CLA.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented May 28, 2026

Hi, there was a checklist when you filled out the PR submission, and one item was

  • I have run and passed the testsuite in CI before submitting the PR, by pushing the changes to my fork and seeing that the automated CI passed there.

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!

Comment thread src/include/OpenImageIO/imageio.h Outdated
/// 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,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented May 28, 2026

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!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants