[plt-dev] Re: [plt] Push #20286: master branch updated

On May 22, Jay McCarthy wrote:
> On Sat, May 22, 2010 at 4:47 AM, Eli Barzilay <eli at barzilay.org> wrote:
> > On May 21, jay at racket-lang.org wrote:
> >> src/foreign/gcc/libffi/src/closures.c
> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> --- OLD/src/foreign/gcc/libffi/src/closures.c
> >> +++ NEW/src/foreign/gcc/libffi/src/closures.c
> >> @@ -379,8 +379,12 @@ dlmmap_locked (void *start, size_t length, int prot, int flags, off_t offset)
> >>         close (execfd);
> >>         goto retry_open;
> >>       }
> >> -      ftruncate (execfd, offset);
> >> -      return MFAIL;
> >> +       if (ftruncate (execfd, offset)) {
> >> +             // FIXME: Should fail "even worse" because the truncate failed
> >> +             return MFAIL;
> >> +      } else {
> >> +         return MFAIL;
> >> +       }
> >
> > Now that I looked at this, it's not clear to me what you're trying to
> > get out of this -- you didn't change anything, and since this is C,
> > there's no real way to "fail worse" besides segfaulting unless you
> > rewrite more code an introduce new special return values etc.
> 
> ftruncate is flagged as "warn if return is ignored" on linux, so if
> the return is not used there's an annoying error. I don't know what
> this code is doing exactly, but presumably it should do "something
> different" if the file truncate fails. By inserting the if, the return
> is handled and the error is gone.

Can you do it in a more obvious way, like

  /* blah blah */
  return ftruncate(...) ? MFAIL : MFAIL;

?  -- this would also make a smaller change from libffi.


> >> src/gracket/gracket.cxx
> >> ~~~~~~~~~~~~~~~~~~~~~~~
> >> --- OLD/src/gracket/gracket.cxx
> >> +++ NEW/src/gracket/gracket.cxx
> >> @@ -19,6 +19,7 @@
> >>  /* wx_motif, for wxTimer: */
> >>  #ifdef __GNUG__
> >>  # pragma implementation "wx_timer.h"
> >> +#pragma GCC diagnostic ignored "-Wwrite-strings"
> >>  #endif
> >
> > I get tons of "warning: ignoring #pragma GCC diagnostic" in the build.
> > (I haven't looked at which platforms, but it's not a single one.)
> 
> That's strange because that's why I put them in __GNUG__. Perhaps it
> is only supported by a newer version of GCC?

For more details (like which platforms this failed on) look at the
build log.


> >> src/gracket/wxs/list.xci
> >> ~~~~~~~~~~~~~~~~~~~~~~~~
> >> --- OLD/src/gracket/wxs/list.xci
> >> +++ NEW/src/gracket/wxs/list.xci
> >>  [...]
> >> -static Scheme_Object *l_MAKE_LIST(l_TYPE l_POINT *f, l_INTTYPE c)
> >> +MAYBE_UNUSED static Scheme_Object *l_MAKE_LIST(l_TYPE l_POINT *f, l_INTTYPE c)
> >
> > This (and many others) looks suspicious.
> 
> Even though Matthew said it's ok. Here's some background: Sometimes
> the xc creates lists and vectors that they only traverse (because
> they came from other files) but they define the same functions many
> times, so gcc complains that you're not using some of the functions
> you define.

The thing that looked suspicious to me is that MAYBE_UNUSED is not
really adding some "maybe_unused" attribute -- but an "unused" one.
If that's fine to add an unused attribute to a function, then
everything's fine.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!