Stefan Fuhrmann wrote on Tue, Jul 30, 2013 at 01:04:43 +0200:
> On Mon, Jul 29, 2013 at 9:06 PM, Daniel Shahaf <danie...@elego.de> wrote:
>
> > danie...@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.
