Fixes for issues reported by OpenScanHub #335
Conversation
zdohnal
force-pushed
the
osh-fixes-v1.4.x
branch
from
349cf93
to
4df6362
Compare
| if (job->state_reasons & PAPPL_JREASON_WARNINGS_DETECTED) | ||
| job->state_reasons |= PAPPL_JREASON_JOB_COMPLETED_WITH_WARNINGS; | ||
| } | ||
| if (job->state == state) |
There was a problem hiding this comment.
I'd prefer to have this be != and put the state change code in the block, rather than adding another exit point from the function...
There was a problem hiding this comment.
@michaelrsweet I've caught this habit from times when I wrote code for a python project - in the end I liked how simple and straight the final code is, instead of closing it in another block.
I can think of a disadvantage being that you have to unlock/free any relevant memory at every exit point. Is there other disadvantage or is it a matter of habit in person's code style?
There was a problem hiding this comment.
@zdohnal If there is no cleanup needed, I do a return (like for the "job" NULL check). But when you have lock (or other) cleanup I either use code blocks or "goto" to make sure that a) I don't have a lot of duplicate code that I have to keep in sync and b) I fully understand the implications of whatever recovery a particular piece of code requires.
| @@ -793,6 +793,8 @@ _papplJobSubmitFile( | |||
| pappl_job_t *job, // I - Job | |||
| const char *filename) // I - Filename | |||
| { | |||
| _papplRWLockRead(job->system); | |||
There was a problem hiding this comment.
This change isn't necessary.
There was a problem hiding this comment.
I've looked into the code more deeply and now I see why it is not necessary - FTR for myself and others - guarding job->format there is unnecessary - every function which calls this function creates its own instance of job, so there are no threads which would change format for the same job at the moment.
No description provided.