On 2017-01-17 19:15:49 -0600, Derek Martin wrote:
> On Sun, Jan 15, 2017 at 11:33:41AM +0100, Simon Ruderich wrote:
> > > else if ((f = fopen (path, "r")) != NULL)
> > > {
> > > struct utimbuf times;
> > > + int ch = 0;
> >
> > The first iteration of the while-loop will initialize ch. I would
> > drop the initialization here as it confused me into thinking the
> > "&& ch" check in the if below verifies if the loop was run at
> > least once. But that will be always true.
>
> I strongly disagree with this: Your confusion notwithstanding, the
> code was perfectly clear. It's much safer and good practice to always
> initialize your variables to a value that's safe.
I disagree. 0 does not have any meaning here, thus it is not safe
('\n' or '\r' would have been better in the context) and it could
lead to bugs. For instance, if the loop were:
while (ch == '\n' || ch == '\r')
{
ch = fgetc(f);
}
then the bug would have remained undetected, while without an
initialization, a good compiler would have detected that the variable
was not initialized. A good practice is to enable warnings that
usually do not yield false positives with good coding style (e.g.
GCC's -Wall, which enables -Wmaybe-uninitialized in particular).
> > > + /* Some mailbox creation tools erroneously append a blank line to
> > > + * a file before appending a mail message. This allows mutt to
> > > + * detect magic for and thus open those files. */
> > > + while ((ch = fgetc(f)) && (ch == '\n' || ch == '\r'));
> >
> > I'd move the ; to th next line to make it more visible or use
> > continue, e.g.
> >
> > while (...)
> > continue;
>
> This one is just a style matter--I feel much less strongly about
> it--but I don't agree with this point either. I've found the empty
> statement is actually a common idiom in a lot of code I've worked on,
> and competent programmers should be able to recognize it. I might
> suggest an additional space before the semi-colon, but the construct
> is fine.
I usually do this:
while (...)
;
or
while (...)
{ }
IMHO, this is as visible as "continue;".
> > ch will only be 0 if there's a NUL-byte in the file which
> > is unexpected for Mboxes but not a problem with the issue the
> > comment is talking about.
>
> It's unexpected but not at all impossible... This patch exists
> entirely to deal with a condition that is not expected. =8^)
NUL bytes can occur when files get corrupted, e.g. due to power outage.
So, they should be detected.
--
Vincent Lefèvre <vinc...@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
