Address review feedback: version-gated NODISCARD_ERROR, fix behavioral changes

mvanhorn · mvanhorn · commit 0c93039cd3f1 · 2026-04-06T08:30:09.000-07:00
- platform.h: Replace simple no-op macro with version-gated
  OIIO_NODISCARD_ERROR_ENABLE that defaults to disabled for OIIO &lt; 3.3
  and enabled for &gt;= 3.3, overridable via -D flag
- py_imagebuf.cpp: Restore returning empty py::bytes() instead of
  py::none() on open failure to preserve backward compatibility
- jpegoutput.cpp: Restructure open/close flow to guarantee close()
  is always called even when open() fails, matching previous behavior
- imageio.h: Apply clang-format to NODISCARD_ERROR-annotated functions

Signed-off-by: Matt Van Horn &lt;455140+mvanhorn@users.noreply.github.com&gt;
diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h
@@ -1415,17 +1415,17 @@ class OIIO_API ImageInput {
     ///                     y, and z).
     /// @returns            `true` upon success, or `false` upon failure.
     ///
-    OIIO_NODISCARD_ERROR virtual bool read_image(int subimage, int miplevel,
-                                           int chbegin, int chend,
-                                           TypeDesc format,
-                                           const image_span<std::byte>& data);
+    OIIO_NODISCARD_ERROR virtual bool
+    read_image(int subimage, int miplevel, int chbegin, int chend,
+               TypeDesc format, const image_span<std::byte>& data);
 
     /// A version of `read_image()` taking an `image_span<T>`, where the type
     /// of the underlying data is `T`.  This is a convenience wrapper around
     /// the `read_image()` that takes an `image_span<std::byte>`.
     template<typename T>
-    OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin,
-                                   int chend, const image_span<T>& data)
+    OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel,
+                                         int chbegin, int chend,
+                                         const image_span<T>& data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_image() does not accept image_span<const T>");
@@ -1438,8 +1438,8 @@ class OIIO_API ImageInput {
     /// contiguous strides in all dimensions. This is a convenience wrapper
     /// around the `read_image()` that takes an `image_span<T>`.
     template<typename T>
-    OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin,
-                                   int chend, span<T> data)
+    OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel,
+                                         int chbegin, int chend, span<T> data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_image() does not accept span<const T>");
@@ -1486,19 +1486,18 @@ class OIIO_API ImageInput {
     /// Added in OIIO 3.1, this is the "safe" preferred alternative to
     /// the version of read_scanlines that takes raw pointers.
     ///
-    OIIO_NODISCARD_ERROR virtual bool read_scanlines(int subimage, int miplevel,
-                                               int ybegin, int yend,
-                                               int chbegin, int chend,
-                                               TypeDesc format,
-                                               const image_span<std::byte>& data);
+    OIIO_NODISCARD_ERROR virtual bool
+    read_scanlines(int subimage, int miplevel, int ybegin, int yend,
+                   int chbegin, int chend, TypeDesc format,
+                   const image_span<std::byte>& data);
 
     /// A version of `read_scanlines()` taking an `image_span<T>`, where the
     /// type of the underlying data is `T`.  This is a convenience wrapper
     /// around the `read_scanlines()` that takes an `image_span<std::byte>`.
     template<typename T>
-    OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin,
-                                       int yend, int chbegin, int chend,
-                                       const image_span<T>& data)
+    OIIO_NODISCARD_ERROR bool
+    read_scanlines(int subimage, int miplevel, int ybegin, int yend,
+                   int chbegin, int chend, const image_span<T>& data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_scanlines() does not accept span<const T>");
@@ -1512,9 +1511,9 @@ class OIIO_API ImageInput {
     /// contiguous strides in all dimensions. This is a convenience wrapper
     /// around the `read_scanlines()` that takes an `image_span<T>`.
     template<typename T>
-    OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin,
-                                       int yend, int chbegin, int chend,
-                                       span<T> data)
+    OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel,
+                                             int ybegin, int yend, int chbegin,
+                                             int chend, span<T> data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_scanlines() does not accept span<const T>");
@@ -4704,6 +4703,6 @@ OIIO_NAMESPACE_END
 
 #if FMT_VERSION >= 100000
 FMT_BEGIN_NAMESPACE
-template<> struct formatter<OIIO::ROI> : ostream_formatter {};
+template<> struct formatter<OIIO::ROI> : ostream_formatter { };
 FMT_END_NAMESPACE
 #endif
diff --git a/src/include/OpenImageIO/platform.h b/src/include/OpenImageIO/platform.h
@@ -462,11 +462,25 @@
 #endif
 
 // OIIO_NODISCARD_ERROR is for functions returning error status (bool) where
-// ignoring the return is a bad practice but not always catastrophic. This is
-// initially a no-op to allow a gentle transition; it can later be defined as
-// [[nodiscard]] once downstream callers have been updated.
-#ifndef OIIO_NODISCARD_ERROR
-#    define OIIO_NODISCARD_ERROR /* nothing for now */
+// ignoring the return is a bad practice but not always catastrophic.
+//
+// OIIO_NODISCARD_ERROR_ENABLE, if nonzero, enables OIIO_NODISCARD_ERROR to
+// take effect. The default is to disable it for OIIO < 3.3, and enable it for
+// OIIO >= 3.3. But `-DOIIO_NODISCARD_ERROR_ENABLE=...` can be used to
+// override the default, for example to flag discarded errors in older
+// versions of OIIO, or to disable the warnings in future versions of OIIO.
+#ifndef OIIO_NODISCARD_ERROR_ENABLE
+#    if OIIO_VERSION_LESS(3, 3, 0)
+#        define OIIO_NODISCARD_ERROR_ENABLE 0 /* disable for now */
+#    else
+#        define OIIO_NODISCARD_ERROR_ENABLE 1 /* enable for OIIO >= 3.3 */
+#    endif
+#endif
+
+#if OIIO_NODISCARD_ERROR_ENABLE
+#    define OIIO_NODISCARD_ERROR OIIO_NODISCARD
+#else
+#    define OIIO_NODISCARD_ERROR /* nothing */
 #endif
 
 
diff --git a/src/jpeg.imageio/jpegoutput.cpp b/src/jpeg.imageio/jpegoutput.cpp
@@ -33,9 +33,7 @@ class JpgOutput final : public ImageOutput {
     ~JpgOutput() override { close(); }
     const char* format_name(void) const override { return "jpeg"; }
     int supports(string_view feature) const override
-    {
-        return (feature == "exif" || feature == "iptc" || feature == "ioproxy");
-    }
+    { return (feature == "exif" || feature == "iptc" || feature == "ioproxy"); }
     bool open(const std::string& name, const ImageSpec& spec,
               OpenMode mode = Create) override;
     bool write_scanline(int y, int z, TypeDesc format, const void* data,
@@ -104,9 +102,7 @@ OIIO_PLUGIN_EXPORTS_BEGIN
 
 OIIO_EXPORT ImageOutput*
 jpeg_output_imageio_create()
-{
-    return new JpgOutput;
-}
+{ return new JpgOutput; }
 
 OIIO_EXPORT const char* jpeg_output_extensions[]
     = { "jpg", "jpe", "jpeg", "jif", "jfif", "jfi", nullptr };
@@ -582,17 +578,14 @@ JpgOutput::copy_image(ImageInput* in)
         close();
         m_copy_coeffs       = (jvirt_barray_ptr*)jpg_in->coeffs();
         m_copy_decompressor = &jpg_in->m_cinfo;
-        if (!open(out_name, orig_out_spec))
-            return false;
-
+        bool ok             = open(out_name, orig_out_spec);
         // Strangeness -- the write_coefficients somehow sets things up
         // so that certain writes only happen in close(), which MUST
         // happen while the input file is still open.  So we go ahead
         // and close() now, so that the caller of copy_image() doesn't
         // close the input file first and then wonder why they crashed.
         close();
-
-        return true;
+        return ok;
     }
 
     return ImageOutput::copy_image(in);
diff --git a/src/python/py_imagebuf.cpp b/src/python/py_imagebuf.cpp
@@ -154,9 +154,7 @@ ImageBuf_setpixel(ImageBuf& buf, int x, int y, int z, py::object p)
 
 void
 ImageBuf_setpixel2(ImageBuf& buf, int x, int y, py::object p)
-{
-    ImageBuf_setpixel(buf, x, y, 0, p);
-}
+{ ImageBuf_setpixel(buf, x, y, 0, p); }
 
 
 void
@@ -194,16 +192,12 @@ ImageBuf_get_pixels(const ImageBuf& buf, TypeDesc format, ROI roi = ROI::All())
 void
 ImageBuf_set_deep_value(ImageBuf& buf, int x, int y, int z, int c, int s,
                         float value)
-{
-    buf.set_deep_value(x, y, z, c, s, value);
-}
+{ buf.set_deep_value(x, y, z, c, s, value); }
 
 void
 ImageBuf_set_deep_value_uint(ImageBuf& buf, int x, int y, int z, int c, int s,
                              uint32_t value)
-{
-    buf.set_deep_value(x, y, z, c, s, value);
-}
+{ buf.set_deep_value(x, y, z, c, s, value); }
 
 
 
@@ -271,7 +265,7 @@ ImageBuf_repr_png(const ImageBuf& self)
     std::unique_ptr<ImageOutput> out = ImageOutput::create("temp.png",
                                                            &file_vec);
     if (!out || !out->open("temp.png", altered_spec))
-        return py::none();
+        return py::bytes();
     self.write(out.get());
     out->close();
 
