subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <danie...@elego.de>
Subject Re: svn commit: r1508170 - in /subversion/trunk: build/ac-macros/svn-macros.m4 configure.ac subversion/include/svn_hash.h
Date Tue, 30 Jul 2013 13:57:51 GMT
Stefan Fuhrmann wrote on Tue, Jul 30, 2013 at 01:04:43 +0200:
> On Mon, Jul 29, 2013 at 9:06 PM, Daniel Shahaf <danielsh@elego.de> wrote:
> 
> > danielsh@apache.org wrote on Mon, Jul 29, 2013 at 18:35:26 -0000:
> > > Author: danielsh
> > > Date: Mon Jul 29 18:35:25 2013
> > > New Revision: 1508170
> > >
> > > URL: http://svn.apache.org/r1508170
> > > Log:
> > > Follow-up to r1507366: svn_hash_gets: compute the length of string
> > literal
> > > keys (common case) at compile-time, without multiply-evaluating
> > dynamically-
> > > computed keys.
> > >
> > > Review by: philip
> > >            breser
> >
> > Oddly enough, the optimization doesn't seem to kick in for me in
> > a '--enable-maintainer-mode' '-C' '--enable-optimize' build.  For some
> > reason my non-maintainer build fails.  Can someone try the following on
> > a release build, please, and report what value they get for klen?
> >
> > % ./libtool --mode=execute gdb -silent -ex r -args subversion/svn/svn
> > update
> > (gdb) b update_internal
> > (gdb) r
> > (gdb) b apr_hash_get
> > Breakpoint 2 at 0x7ffff6b25ac0: file tables/apr_hash.c, line 342.
> > (gdb) c
> > Continuing.
> >
> > Breakpoint 2, apr_hash_get (ht=0x65c698, key=0x7ffff7bcf86c, klen=-1)
> >     at tables/apr_hash.c:342
> > 342     {
> > (gdb) up
> > #1  0x00007ffff7bccd69 in update_internal (result_rev=0x7fffffffe260,
> >     conflicted_paths=<value optimized out>,
> >     local_abspath=0x6987e0 "/home/danielsh/src/svn/t1",
> >     anchor_abspath=0x698a60 "/home/danielsh/src/svn/t1",
> >     revision=0x7fffffffe170, depth=svn_depth_unknown, depth_is_sticky=0,
> >     ignore_externals=0, allow_unver_obstructions=0, adds_as_modification=1,
> >     timestamp_sleep=0x7fffffffe26c, notify_summary=1, ctx=0x65d810,
> >     pool=0x6986a8) at subversion/libsvn_client/update.c:240
> > 240                           ? svn_hash_gets(ctx->config,
> > SVN_CONFIG_CATEGORY_CONFIG)
> > (gdb)
> >
> > I would expect 6, since SVN_CONFIG_CATEGORY_CONFIG is known at
> > compile-time.
> >
> 
> A two things have been broken here:
> 

Thanks for fixing this.  Collective review of your commits:

r1508221: should update the docstring of SVN_HAS_DUNDER_BUILTINS in
configure.ac to remove mention of __builtin_choose_expr

r1508221: should update the AC_COMPILE_IFELSE call in the
cross-compilation branch to remove __builtin_choose_expr

r1508222: the #ifndef seems to be the wrong solution to whatever problem
it is trying to fix.  If it's trying to generate a compiler warning, it
should use the #warning preprocessor directive; if it's trying to
silence one about use of an undefined macro, how about doing
  #ifndef SVN_HAS_DUNDER_BUILTINS
  #error "Build broken (do you need to include svn_private_config.h earlier?)"
  #endif
and adding a
  #define SVN_HAS_DUNDER_BUILTINS 0
to subversion_config_private.hw?

r1508225: changing the include order //only in places that currently
include svn_hash.h// seems like a bad idea; we should change it
_everywhere_ (since other places may grow svn_hash.h includes in the
future).

Also, let's take this opportunity to document our preferred order of
#include's in HACKING (The relative order of: stdlib, apr, mod_dav, bdb,
serf, sqlite, svn_*.h, svn_*_private.h, ./*.h, ../libsvn_{fs|ra}/*.h,
subversion_config_private.h, and APR_WANT_FOO) --- I always end
up copying the order from some existing file.

> * the ac macro (fixed in r1508221)
> * the definition / #include order (detected as of r1508222, fixed in
> r1508225)
> 
> Now, I see the optimization work on my machine again.
> 

Great, thanks for finishing what I'd started!

> -- Stefan^2.

Mime
View raw message