Prevent extra EPS header validations

[Yay295]

This is the only return statement in this function, and it appears to be unnecessary since breaking from the loop at this location accomplishes the same thing, and matches what the rest of this loop does.

The only change is that check_required_header_comments() is called at the end of the function. This was being skipped before due to the early return, but it will be called now that it's just breaking from the loop.

Before 8f75cc2 this return was inside a nested loop, so breaking wouldn't have worked, but now it does. Also, check_required_header_comments() didn't exist at that time, so the early return wasn't skipping anything.

[radarhere]

The only change is that check_required_header_comments() is called at the end of the function. This was being skipped before due to the early return, but it will be called now that it's just breaking from the loop.

It's already been called by this point.

reading_header_comments starts out as true.

Pillow/src/PIL/EpsImagePlugin.py

Line 228 in 6a6ad65

reading_header_comments = True

The image data part of the code can't be reached until reading_header_comments is false

Pillow/src/PIL/EpsImagePlugin.py

Line 300 in 6a6ad65

if reading_header_comments:

Pillow/src/PIL/EpsImagePlugin.py

Line 327 in 6a6ad65

elif bytes_mv[:11] == b"%ImageData:":

But it is only set to false after check_required_header_comments() is called.

Pillow/src/PIL/EpsImagePlugin.py

Lines 291 to 292 in 6a6ad65

	check_required_header_comments()
	reading_header_comments = False

Pillow/src/PIL/EpsImagePlugin.py

Lines 307 to 308 in 6a6ad65

	check_required_header_comments()
	reading_header_comments = False

If anyone is looking at this and thinks 'What about the size check?'

Pillow/src/PIL/EpsImagePlugin.py

Lines 370 to 372 in 6a6ad65

	if not self._size:
	msg = "cannot determine EPS bounding box"
	raise OSError(msg)

That has to be set by the end of the image data part of the code.

Pillow/src/PIL/EpsImagePlugin.py

Line 356 in 6a6ad65

self._size = columns, rows

So the change suggested here doesn't make any functional difference.

[Yay295]

So actually, if check_required_header_comments() was added at line 273, I think it could be removed from the end.

[radarhere]

Ok, you're describing

diff --git a/src/PIL/EpsImagePlugin.py b/src/PIL/EpsImagePlugin.py
index 380b1cf0e..20ef0e5e2 100644
--- a/src/PIL/EpsImagePlugin.py
+++ b/src/PIL/EpsImagePlugin.py
@@ -270,6 +270,7 @@ class EpsImageFile(ImageFile.ImageFile):
             if byte == b"":
                 # if we didn't read a byte we must be at the end of the file
                 if bytes_read == 0:
+                    check_required_header_comments()
                     break
             elif byte in b"\r\n":
                 # if we read a line ending character, ignore it and parse what
@@ -365,8 +366,6 @@ class EpsImageFile(ImageFile.ImageFile):
                 trailer_reached = True
             bytes_read = 0
 
-        check_required_header_comments()
-
         if not self._size:
             msg = "cannot determine EPS bounding box"
             raise OSError(msg)

Sure, that looks correct, if you like it as a fractional performance increase.

[radarhere]

I think your first commit could be removed? As I was saying, it doesn't make any functional difference. It just unnecessarily runs the check(s) at the end.

[Yay295]

I think it makes the function easier to read by only having continues and breaks, and not having one early return.

[radarhere]

I don't personally agree, but we can see what others think.

[wiredfool]

I'm generally a fan of early exit rather than introducing conditional checks to allow a method to run to completion when there shouldn't be code executed anyway.

[Yay295]

I would normally agree, but in this case this is the only return in the method, and it's inside a loop, so it's easy to miss it, and this change also stops the check from happening twice in some situations.

[radarhere]

this change also stops the check from happening twice in some situations.

As per my earlier comment, the return change doesn't have any functional impact. Your second commit on the other hand, does stop check_required_header_comments() from potentially being called twice.

[Yay295]

I still think it's bad form to have that return there, but I've removed the first commit and rebased on main.