DanBloomberg / leptonica

Leptonica is an open source library containing software that is broadly useful for image processing and image analysis applications. The official github repository for Leptonica is: danbloomberg/leptonica. See leptonica.org for more documentation.
Other
1.74k stars 387 forks source link

(feature) append filename to any error messages where this would be useful #682

Closed GerHobbelt closed 1 year ago

GerHobbelt commented 1 year ago

append filename to any error messages where this would be useful (and available), so the user can locate the culprit more easily.

DanBloomberg commented 1 year ago

This is an interesting patch. I like framing it generally as a 2nd argument (the 1st being the problem, the 3rd the procedure name and the 4th the return value.

One question: Does this modify all the instances in the library where an image file is being read?

One observation: this does not need to be used in programs in the prog directory. (You may have included those 2 instances for testing?) For regression programs, we have sufficient information about failures when they occur. And other programs are relatively short and if they fail, debugging can easily be done if necessary.

DanBloomberg commented 1 year ago

Actually, I answered my own question: grep fopenReadStream .c | wc 58 400 3418 grep fopenWriteStream .c | wc 45 347 2981

We have over 100 files and 700 instances where these new macros could be used for reading and writing files. How did you pick the subset that you modified?

GerHobbelt commented 1 year ago

How did you pick the subset that you modified?

😅 😊 I hadn't thought to apply grep to those calls. I came in via a search for the actual error messages, that were not informative enough for me & my memory (I like to see what happens and who might have caused it right away, even in small apps), and then expanded within the located functions from there. Tried to do a rigorous job, but clearly failed at that. 😞

This is an interesting patch. I like framing it generally as a 2nd argument (the 1st being the problem, the 3rd the procedure name and the 4th the return value.

Yup, that's a nice generalization. (Though I did make that filename/path the last argument when patching a few L_ERROR() calls, IIRC, but I didn't include those in the cherrypick/filtering of original 'Mother-of-All' commit 90db7964d09b5f58c3ca590a74c87b5ba969d50a.)

GerHobbelt commented 1 year ago

New commit added to the pullreq. Did a search + patch for the missing items following your https://github.com/DanBloomberg/leptonica/pull/682#issuecomment-1477343352 -- I hadn't searched for "image file not found" previously, which would've caught 98% of these. Whoops!

One question: Does this modify all the instances in the library where an image file is being read?

I did my best (which wasn't good enough before 😅 ) to answer 'aye!'


One observation: this does not need to be used in programs in the prog directory. [...]

Yes, and No. When I was fiddling with those as part of getting alltests to run in my particular build, having those filenames in the collective stderr output saved me a bundle of time discovering my patching + 'copy those /prog/*image files to a suitable test directory near the compiled binaries' Makefile/script had some noticable flaws. But then I was doing things to /prog/*.c that you never intended, so... it depends. 😉

Besides, I like my error/diagnostics info a little verbose, so I don't have to go dig in the code (or remember what we do exactly right now or where we are in the run-time) if I don't have to. Saves me time, but that's another 'it depends'. Some people I have worked with get nauseous when they see a log/diag output zipping by; I don't.

DanBloomberg commented 1 year ago

Thank you very much for this improvement in the leptonica infrastructure.