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

[olafBuddenhagen]

Hi,

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.)

> > > +      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 :-)

> > Also, I wonder whether the macro would break if you do:
> > 
> >    opt_name = OPT_LONG(transparent_mount ? OPT_LONG_MOUNT : 
> > OPT_LONG_NOMOUNT)
> 
> I had tried this before posting the patch; it breaks.
> 
> Take a look at the definition of OPT_LONG:
> 
> #define OPT_LONG(o) "--" o

I see.

-antrik-