Re: PATCH 1/2 - fix all compiler warnings. (was XMLFS for GSoC)

[Michael Walker]

> That's always a good start!  Where is this repository that your patch is
> against?

http://cvs.savannah.gnu.org/viewvc/xmlfs/?root=hurdextras

> Your editor / mailer broke the lines, so this patch won't even apply.
> Either you instruct your editor / mailer, or let git send-email take care
> of that, or attach the patches instead of publishing them inline.

Heh, oops. I forgot claws-mail strictly enforces line length limits.
I'll keep that in mind

> Hrm.  Not sure whether we'd like all of these.  But you're (to the best
> of my knowledge) to only person working on xmlfs these days, and we have
> no general template, so I'd let you decide.

If any of them turn out to be a problem, or overly complicate the code
(as pretty much everything needs to be explicitly stated with those
flags) I'll consider removing some of them.

> If we really want this, wouldn't it be better to use ``__attribute__
> ((unused))'' with each of the functions' parameters?  In Hurd code, we
> can unconditionally use GNU C / GCC features.

I didn't know of that attribute, and I think I will start using
GCC/GNU stuff more (dropping -pedantic), as GCC is the compiler used
(as antrik pointed out in another email)

> I don't know the surrounding code yet, but should gsize perhaps by a
> size_t instead?

Probably, a lot of the 'problems' could undoubtedly have been fixed in
other ways.

> memcpy with length of one?  Why not replace that with:
>
>    data[size++] = '\n';
>
> (Your cast of data doesn't look right anyway; do you understand and
> agree?)  I should have a look at the whole function -- it still looks a
> bit suspicious: what will come after the '\n' charater; is a '\0'
> expected there?

Yes, that would be a much better solution. I didn't really read the
code thoroughly enough when fixing the warnings, usually going for the
most obvious solution (which may turn out to have caused some issues
later down the line, I suppose). And yes, the case of data looks
completely wrong *facepalm*

> Why move these functions?  Typically, all definitions should be as tight
> in scope as possible.

Warnings about nested functions, though if I'm going to drop -pedantic
and use GCC extensions, that doesn't matter.

> That doesn't look sane.  According to netfs.h these indeed aren't const,
> but why?  They're used only in libnetfs/io-version.c.

I'm not sure. I assumed there was a reason for libnetfs not having
them as consts.

> Both openport and open return an int file descriptor, so why is xmlfile a
> file_t (which is another name for a mach_port_t)?

Looks like xmlfs_create (in fs.c) is using a file_t as a file
descriptor. As that's clearly wrong I'll change that.

>> @@ -114,7 +118,7 @@ main (int argc, char **argv)
>>
>>    netfs_root_node->nn_stat = underlying_stat;
>>    netfs_root_node->nn_stat.st_mode =
>> -    S_IFDIR | (underlying_stat.st_mode & ~S_IFMT & ~S_ITRANS);
>> +    S_IFDIR | (underlying_stat.st_mode & (unsigned int) ~S_IFMT &
>> (unsigned int) ~S_ITRANS);
>
> What's the warning here?

Implicit casting to unsigned int.

Thanks for the comments.

-- 
Michael Walker (http://www.barrucadu.co.uk)

Arch Hurd Developer;      GNU Webmaster;       FSF member #8385
http://www.archhurd.org   http://www.gnu.org   http://www.fsf.org