Repository navigation
Remove conditionally-compiled or unreachable debug C code - #9915
Conversation
akx
changed the title
NDEBUG is "not debug"; the debug checks were _enabled_ when it was set
wiredfool
left a comment
wiredfool
left a comment
There was a problem hiding this comment.
So I'm seeing that we've got
- multiple different conditional compliation flags for c level trace debugging.
- Personally the tiff stuff was necessary when putting that together, but it's probably outlived 90% of it's usefulness. Especially since it borks the terminal when dumping under python3, but presumably that's fixable.
- An actual misunderstanding of the NDEBUG flag
- One random extra free.
Instrumentation like what's in the tiff bits is useful for seeing a big picture where debuggers don't give it.
I'm not going to object, at least assuming I don't have to add stuff like this back in.
| printf("Oops, split failed...\n"); | ||
| #endif | ||
| exit(1); | ||
| ImagingQuantHeapFree(h); |
There was a problem hiding this comment.
This looks like something more than just a dead code removal.
There was a problem hiding this comment.
Yes, that's described in the PR message and commit message.
on top of that, there's a fun commit that removes an exit(1); from an unlikely error branch of Quant.c. I suppose Quant.c was ported from some application code in 1998 and no one noticed all of the exit()s.
|
|
||
| dib->palette = CreatePalette(pal); | ||
| } else if (mode == IMAGING_MODE_RGB) { | ||
| #ifdef CUBE216 |
There was a problem hiding this comment.
I assume this can never be set?
There was a problem hiding this comment.
As far as I've trawled through the Git history, it has never been set. You would've needed to know about this undocumented preprocessor variable, and compiled the library with it by hand, to get a different automatic palette for DIB exports.
The obvious way is to convert your RGB image to P mode in Pillow code with a palette of your heart's desire, and then DIB it, rather than have this code (from PIL 1.1.1, Oct 20 2000, if my archaeology is correct) guess a palette.
Yep, I'd assume people working with hairy code like the TIFF or quantization stuff would add their own prints in their working copy. (I know that's what I've done with stuff like #9829 and the like.) But those probably don't belong in Thanks for taking a look! |
|
|
||
| #else | ||
|
|
||
| /* Colour DIB. Alternate palette. */ |
There was a problem hiding this comment.
What was your thinking behind removing 'Alternate palette'?
There was a problem hiding this comment.
Noted above in #9915 (comment) – never documented, never mentioned otherwise in Git history, you'd have to compile PIL from source on Windows to use this.
There was a problem hiding this comment.
And for the phrasing... well, it's not "alternative" because there's no alternative anymore?
There was a problem hiding this comment.
Yes, I wasn't sure what the 'alternate' was referring to, but your thinking is that it referred to CUBE216. Fair enough, thanks.
| #include <stdlib.h> | ||
| #include <memory.h> | ||
| #include <time.h> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
Gives us UINT32_MAX without having to define it by hand below.
* Do not pass TIFF compression to C * Restore debugging line as comment --------- Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>
|
Thanks for the merge! ❤️ |
I was looking at
Mode.cand noticed 0567f06 had gotten the semantics ofNDEBUGwrong;NDEBUGis true when not debugging, so these checks were running all the time in release builds. That said, those checks weren't really necessary anyway; we can see that all of the constantModeDatain that file has a non-NULL mode string. It's not worth testing at runtime.Following that, I took a look at other
#ifdef'd code to get rid of other debug-like code that was never compiled in... and on top of that, there's a fun commit that removes anexit(1);from an unlikely error branch ofQuant.c. I supposeQuant.cwas ported from some application code in 1998 and no one noticed all of theexit()s.