Re: [PATCH 1/4] Add the ``--no-mount'' option.

[Sergiu Ivanov]

Hello,

On Sat, Aug 08, 2009 at 04:53:29AM +0200, olafBuddenhagen@gmx.net wrote:
> On Mon, Aug 03, 2009 at 10:39:51PM +0300, Sergiu Ivanov wrote:
> > On Wed, Jul 29, 2009 at 09:27:10AM +0200, olafBuddenhagen@gmx.net wrote:
> > > On Fri, Jul 17, 2009 at 01:56:57PM +0300, Sergiu Ivanov wrote:
> 
> > > > +/* Shows the mode in which the current instance of unionmount
> > > > +   operates (transparent/non-transparent).  */
> > > > +int transparent_mount = 1;
> > > 
> > > I think it would be clearer to default to "0" and set it on --mount...
> > > 
> > > But that's not terribly important really :-)
> > 
> > I set it to 1 because in this case argp_parse_common_options requires
> > only the addition of the lines for handling the OPT_NOMOUNT option,
> > which resumes to assigning 0 to transparent_mount.  Setting this
> > variable to 0 initially will require adding one more line and several
> > line shuffles.
> 
> Not at all. Unless I'm missing something crucial, you can use exactly
> the same method -- only doing =1 in the OPT_MOUNT case instead of =0 for
> OPT_NOMOUNT.
> 
> (Of course, you also have to switch the cases, as it's the other one
> that needs a fallthrough when doing it this way around.)

This is what I meant when I was talking about ``line shuffling'' :-)
 
> > > > +      char * opt_name = (transparent_mount ? OPT_LONG (OPT_LONG_MOUNT)
> > > > +                        : OPT_LONG (OPT_LONG_NOMOUNT));
> > > 
> > > Don't mix declarations and statements. While C99 allows this, and gcc
> > > supported it even before, it's not very good coding style IMHO. I
> > > haven't seen it in other Hurd code.
> > > 
> > > (There is a single instance of "for (int i=..." in rpctrace; but even
> > > that is questionable -- and it's not quite the same thing anyways...)
> > 
> > OK, thank you for explanation.  I had never used this style before and
> > wanted to ``try'' it out.
> >  
> > > I don't remember whether GCS says something on that?
> > 
> > I skimmed the ``Making the Best Use of C'' section and didn't notice
> > anything (though a more attentive perusal might reveal something).
> > Anyways, I've never seen variables initialized at declaration in the
> > Hurd, so I won't do like that.
> 
> That's not what I'm talking about here. The GCS *does* say not to
> initialize variables at declaration time (or at least it did a couple of
> years ago) -- but some existing Hurd code actually ignores this
> recommendation, so I don't really consider it a problem.
> 
> What I'm talking about here is that you have a declaration in the middle
> of a code block, between statements. This is not allowed in C++ and in
> C99, but not in traditional C, where all declarations have to be at the
> beginning of a block.
> 
> So moving the whole thing up would be sufficient to address the issue I
> was actually talking about -- but splitting it up like you did in the
> updated patch is also fine of course :-)

Fine :-) I'll stick with splitting declaration and initialization,
since it (usually) makes the code more readable (by splitting it into
something like ``segments'').
 
Regards,
scolobb