On Tue, Jul 30, 2013 at 3:57 PM, Daniel Shahaf <danielsh_at_elego.de> wrote:
> 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_at_elego.de>
> wrote:
> >
> > > danielsh_at_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
Fixed in r1508458.
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
>
No go. This is a public API header. People will not define
SVN_HAS_DUNDER_BUILTINS in their applications.
> and adding a
> #define SVN_HAS_DUNDER_BUILTINS 0
> to subversion_config_private.hw?
>
Done in r1508460,
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).
>
Well, I got kind of fed up after manually patching
~180 files (about the same number of files left to
check). Even the commit took longer than 4 minutes.
So, feel free to do some search/replace magic to
handle the remaining files.
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!
>
Thanks for the review!
-- Stefan^2.
Received on 2013-07-30 16:30:33 CEST