Re: [PATCH] Use the new __hurd_exec_file_name RPC

[Carl Fredrik Hammar]

Hi,

On Tue, Jun 01, 2010 at 03:50:26AM +0200, olafBuddenhagen@gmx.net wrote:
> On Mon, May 31, 2010 at 06:16:06PM +0200, Carl Fredrik Hammar wrote:
> > On Sat, May 22, 2010 at 06:26:29PM +0200, Emilio Pozuelo Monfort wrote:
> 
> > > +      err = __file_exec_file_name (file, task, flags,
> > > +                            filename ? filename : "",
> > > +                            args, argslen, env, envlen,
> > > +                            dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> > > +                            ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
> > 
> > Break this line.
> 
> Hm... Does glibc have a strict 80-chars-or-die policy? I always
> considered this ridiculous...

I don't think it's strict as there are examples of such code in glibc.
I might be projecting my own aversion to long lines here, so tell me if
you think I'm being too strict.

> > > + error_t err = __file_exec_file_name (file, task,
> > > +                                      (__sigismember (&_hurdsig_traced, 
> > > SIGKILL)
> > > +                                       ? EXEC_SIGTRAP : 0),
> > > +                                      filename,
> > > +                                      args, argslen, env, envlen,
> > > +                                      dtable, MACH_MSG_TYPE_COPY_SEND, 
> > > dtablesize,
> > > +                                      ports, MACH_MSG_TYPE_COPY_SEND, 
> > > _hurd_nports,
> > > +                                      ints, INIT_INT_MAX,
> > > +                                      NULL, 0, NULL, 0);
> > 
> > A bunch of lines should be broken here, but the first one is particularly
> > tricky.  Do something like this instead:
> > 
> >     error_t err = __file_exec_file_name
> >       (file, task,
> >        __sigismember (&_hurdsig_traced, SIGKILL) ? EXEC_SIGTRAP : 0,
> >        filename,
> >        args, argslen, env, envlen,
> >        dtable, MACH_MSG_TYPE_COPY_SEND, dtablesize,
> >        ports, MACH_MSG_TYPE_COPY_SEND, _hurd_nports,
> >        ints, INIT_INT_MAX,
> >        NULL, 0, NULL, 0);
> 
> Not sure it's a good idea to be inconsistent here. I there precedent for
> this?

Yes, see exec/exec.c:1510 in Hurd and libidn/stringprep.c:192 in glibc.
I actually think this style should be used more for huge function calls
like this.  It makes the indentation less "jumpy".

> BTW, how about breaking after the '='? I think I saw this in other code
> -- though I don't know in what projects...

That is also an option though I think the above is better in this case
since "error_t err =" is shorter than "__file_exec_file_name" so you
don't gain as much room.  Interestingly, indent does both breakings in
this case but that seems unnecessary.

I also think I have seen code that breaks after the opening parenthesis,
but that seems less gnuish.

Regards,
  Fredrik