Add Replace RG check exit and add some comments#2254
Conversation
mp15
commented
Sep 4, 2025
- Check for unclean file close in bam_addrprg.c and set EXIT_FAILURE if that happens.
- Add comments to bam_addrprg.c and ensure non-trivial ifs are explicitly {}'d
| if (opts->uncompressed) { | ||
| strcat(output_mode, "0"); | ||
| if (opts->output_name) // File format auto-detection | ||
| } | ||
| if (opts->output_name) { // File format auto-detection | ||
| sam_open_mode(output_mode + strlen(output_mode), | ||
| opts->output_name, NULL); | ||
| } |
There was a problem hiding this comment.
You mention "ensure non-trivial ifs are explicitly {}'d". However both of these are single statements rather than compound ones.
Do you define a trivial if as one on the same line, rather than this?
if (foo)
bar();
Generally our coding style is single-statements don't require bracing. I don't object if people do it for new code as it's a minor thing, but I don't see the benefit of changing old code to be that like, especially when there are other single statement if's left unchanged, eg your own if (!cleanup_state) below.
There was a problem hiding this comment.
to be fair the change to if (opts->uncompressed) I'm happy to drop. It was the multiline nature of sam_open_mode that gave me the urge to {} it
There was a problem hiding this comment.
Don't let my comments about style seem that I'm ungrateful though. Thank you for the changes. (If you don't have time then I'm sure one of us could reformat in a more house-style during merge if you wish. Plus fixing the trailing spaces from debian-gcc check)
4edb8e0 to
6bbfa7f
Compare
|
I've tweaked the code a lil bit to address your points. |
6bbfa7f to
7492a00
Compare
- Check for unclean file close in bam_addrprg.c and set
EXIT_FAILURE if that happens.
- Add comments to bam_addrprg.c and ensure ifs are explicitly {}'d
7492a00 to
d9b3ef3
Compare