api: add OIIO_NODISCARD_ERROR to ImageOutput methods by rose413 · Pull Request #5201 · AcademySoftwareFoundation/OpenImageIO

rose413 · 2026-05-16T00:29:40Z

Description

Annotate all ImageOutput write methods (write_image, write_scanline, write_scanlines, write_tile, write_tiles, write_rectangle, write_deep_scanlines, write_deep_tiles, write_deep_image), as well as open, close, and copy_image, with OIIO_NODISCARD_ERROR.
Also fix the handful of internal callsites (imagebuf.cpp, maketexture.cpp, py_imagebuf.cpp, iconvert.cpp, imagebufalgo_test.cpp, and imagespeed_test.cpp) that were previously ignoring the return value of close(), either by propagating the error or by casting to void where the error path is already being handled.
Assisted-by: Claude Sonnet 4.6
Reference: [FEATURE REQUEST] Add [[nodiscard]] to image read/write methods #4781

Tests

tested against unit_imageinout, unit_imagebuf, unit_imagebufalgo

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.

- Annotate all ImageOutput write methods (write_image, write_scanline, write_scanlines, write_tile, write_tiles, write_rectangle, write_deep_scanlines, write_deep_tiles, write_deep_image), as well as open, close, and copy_image, with OIIO_NODISCARD_ERROR. - Also fix the handful of internal callsites (imagebuf.cpp, maketexture.cpp, py_imagebuf.cpp) that were previously ignoring the return value of close(), either by propagating the error or by casting to void where the error path is already being handled. - Assisted-by: Claude Sonnet 4.6 Signed-off-by: rose413 <116857309+rose413@users.noreply.github.com>

linux-foundation-easycla · 2026-05-16T00:29:46Z

The committers listed above are authorized under a signed CLA.

✅ login: rose413 / name: rose413 (3ead3a8)

lgritz · 2026-05-16T02:07:35Z

The failure for the "VFX2023" test is unrelated to your PR and is being fixed separately. If everything else is ok, we will not hold up the merge just because of that.

lgritz

This is looking really good.

I made some suggestions -- I think a few of the places in tests where you ignore the results, probably it would be better if we checked, and exit the loops early if there is a problem.

Oh, I guess in my explanations, I showed how to detect and exit the loop, but then didn't handle it. For tests, you don't need nice error recovery, but maybe just do a OIIO_CONTRACT_ASSERT(ok) after the loop?

lgritz · 2026-05-16T02:00:53Z

    bool ok = out->open(output_filename, outspec);
    OIIO_ASSERT(ok);
-    out->write_image(bufspec.format, &buffer[0]);
+    (void)out->write_image(bufspec.format, &buffer[0]);


Maybe?

Suggested change

(void)out->write_image(bufspec.format, &buffer[0]);

ok = out->write_image(bufspec.format, &buffer[0]);

OIIO_ASSERT(ok);

I wrote OIIO_ASSERT, but probably OIIO_CONTRACT_ASSERT(ok) is the "more modern" idiom we're aiming for most of the time.

lgritz · 2026-05-16T02:02:16Z

    imagesize_t scanlinesize = outspec.width * pixelsize;
    for (int y = 0; y < outspec.height; ++y) {
-        out->write_scanline(y + outspec.y, outspec.z, bufspec.format,
+        (void)out->write_scanline(y + outspec.y, outspec.z, bufspec.format,


Maybe

bool ok = true; for (int y = 0; y < outspec.height && ok; ++y) { ok = out->write_scanline(y + outspec.y, outspec.z, bufspec.format, &buffer[scanlinesize * y]); }

lgritz · 2026-05-16T02:06:23Z

-                             z + outspec.z, z + outspec.z + outspec.tile_depth,
-                             bufspec.format, &buffer[scanlinesize * y],
-                             pixelsize /*xstride*/, scanlinesize /*ystride*/);
+            (void)out->write_tiles(outspec.x, outspec.x + outspec.width,


lgritz · 2026-05-16T02:06:27Z

-                                bufspec.format,
-                                &buffer[scanlinesize * y + pixelsize * x],
-                                pixelsize, scanlinesize, planesize);
+                (void)out->write_tile(x + outspec.x, y + outspec.y, z + outspec.z,


Same here, maybe more like

bool ok = true; for (int z = 0; z < outspec.depth && ok; ...) { for (int y = 0; y < outspec.height && ok; ...) { for (int x = 0; y < outspec.width && ok; ...) { ok = out->write_tile(...)

I guess I'm superstitious about ignoring the return code, that's what we're trying to be caching. Even though this is a test... what better time to catch a problem then when we're testing?

…rmat

lgritz · 2026-05-17T01:23:57Z

I have a feeling that you did a reformat of code you weren't really trying to change, and you must have a different version of clang-format locally than we are using in CI.

rose413 · 2026-05-19T02:53:35Z

What would you recommend I do to make sure I’m using the same clang-format version as CI?

lgritz · 2026-05-19T05:13:24Z

What would you recommend I do to make sure I’m using the same clang-format version as CI?

I don't, really. We tend to lock down to a clang-format version in CI for a few years at a time to make a stable standard, so that we're not constantly reformatting with every clang-format update. But of course, that means that for those of us who keep our own machines fairly up to date, we can drift from the CI standard. (Of course you COULD install the clang-format 17 we are using in CI but it's probably not worth the trouble.)

So first, I never run a clang-format of the full code base on my machine (for this reason -- it will change too many things I wasn't even working on). I use VSCode and so usually I'll just highlight/select the function I altered, or sometimes just the lines I changed, and ask it to reformat that selection only, rather than the whole file or the whole codebase. That at least limits any changes to just the sections I was intending to edit.

When my CI fails the clang-format test, I just look at the logs for that run, which in the "clang-format" section, will show the diffs between the actual code and the CI-formatted version, as you can see here. USUALLY it only differs on a couple lines and I just edit those by hand.

It's usually easy to see exactly what to do from the diffs in the log. But when it's more extensive or hard to do by hand, I check the downloadable artifacts (click "Summary" on the upper left of the CI dashboard, then scroll all the way to the bottom, where you'll see downloads for each job, and one will be called clang-format). That saved artifact is a zip file of the source code with proper formatting, so you can extract the files you need formatted precisely the way the CI thought it should be done.

I'm not sure what tools you have on your end, but what I'd do first if I were you is try to revert all the places in your last commit that reformatted parts of the code you didn't intend to change at all. Then you will only have the small number of differences in the code you really did modify. That should be manageable by hand or by downloading the changed files.

Also, if you just can't figure it out, I am happy to fix the formatting for you on my end, and amend your PR with the fixes

lgritz · 2026-05-19T05:14:01Z

Oh, one more thing -- it looks like at least one of your commit did not use git commit -s to affix signed-off-by line that certifies your acceptance of the DCO.

…rmat Signed-off-by: rose413 <116857309+rose413@users.noreply.github.com>

Signed-off-by: rose413 <116857309+rose413@users.noreply.github.com>

lgritz · 2026-05-19T06:47:12Z

Aha, looks like you got all the formatting fixed!

lgritz · 2026-05-19T15:45:45Z

+        errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());
+    }


I think you should also set ok=false so that the function's return value indicates that something went wrong and that we don't overwrite the real file (at this point we are writing to a temporary file, and will, later i the function, replace the destination file only if everything appeared to go right).

Suggested change

errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());

}

errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());

ok = false;

}

lgritz · 2026-05-19T15:53:46Z

    size_t pixelsize         = outspec.nchannels * sizeof(float);
    imagesize_t scanlinesize = outspec.width * pixelsize;
+    bool ok                  = true;
    for (int y = 0; y < outspec.height; ++y) {


You assign the result to ok, but ok isn't actually used anywhere. I think that it would be better if you had an OIIO_ASSERT(ok) (or OIIO_CONTRACT_ASSERT, I guess) after the loop. And the loop should probably terminate as soon as there's an error, say:

for (int y = 0; y < outspec.height && ok; ++y)

This file is a bunch of unit tests, so it's not important to issue meaningful error messages with errorfmt or anything like that. But it is a test, so we should at least have an assertion to let us know that it's not really successfully testing the thing that we think it's testing.

I'm only writing this comment here, but I think all of the changes in this file should follow a similar pattern.

lgritz

We are very close. Please see my comments about at least asserting success for the changes to the tests.

lgritz requested changes May 16, 2026

View reviewed changes

rose413 added a commit to rose413/OpenImageIO that referenced this pull request May 17, 2026

Fix review comments in AcademySoftwareFoundation#5201; apply clang-fo…

6b292bb

…rmat

lgritz mentioned this pull request May 19, 2026

fix(pnm,jxl): Prevent PNM/JXL readers from loading arbitrarily large non-image files #5203

Merged

7 tasks

Fix review comments in AcademySoftwareFoundation#5201; apply clang-fo…

e092be1

…rmat Signed-off-by: rose413 <116857309+rose413@users.noreply.github.com>

rose413 force-pushed the nodiscard-imageio-h branch from 6b292bb to e092be1 Compare May 19, 2026 06:07

style: apply clang-format-17 fixes

ac22aaa

Signed-off-by: rose413 <116857309+rose413@users.noreply.github.com>

lgritz reviewed May 19, 2026

View reviewed changes

lgritz requested changes May 19, 2026

View reviewed changes

lgritz assigned rose413 May 19, 2026

lgritz added core APIs Affecting public APIs of core functionality classes, such as ImageInput, ImageOutput, ImageBuf. devdays26 Dev Days 2026 labels May 19, 2026

	(void)out->write_image(bufspec.format, &buffer[0]);
	ok = out->write_image(bufspec.format, &buffer[0]);
	OIIO_ASSERT(ok);

		errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());
		}

Conversation

rose413 commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Checklist:

Uh oh!

linux-foundation-easycla Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgritz commented May 16, 2026

Uh oh!

lgritz left a comment

Choose a reason for hiding this comment

Uh oh!

lgritz May 16, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz May 16, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz May 16, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz May 16, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz May 16, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz commented May 17, 2026

Uh oh!

rose413 commented May 19, 2026

Uh oh!

lgritz commented May 19, 2026

Uh oh!

lgritz commented May 19, 2026

Uh oh!

lgritz commented May 19, 2026

Uh oh!

lgritz May 19, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz May 19, 2026

Choose a reason for hiding this comment

Uh oh!

lgritz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rose413 commented May 16, 2026 •

edited

Loading

linux-foundation-easycla Bot commented May 16, 2026 •

edited

Loading