Stefan Fuhrmann wrote on Tue, Jul 30, 2013 at 16:30:00 +0200:
> 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
...
> > > >
> > >
> > > A two things have been broken here:
> > >
> >
> > Thanks for fixing this. Collective review of your commits:
> >
> > 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.
>
It would be nice if we can propagate the fix to C API consumers too.
How can we do that? Do we just document the macro (say, in an #ifdef
DOXYGEN block) along with an example of how to detect the setting for
it in various build systems? Or do try to sniff it at compile time?
> > 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.
What makes you think I'm not as lazy as you are? :-)
Index: build/ac-macros/compiler.m4
===================================================================
--- build/ac-macros/compiler.m4 (revision 1508471)
+++ build/ac-macros/compiler.m4 (working copy)
@@ -74,6 +74,7 @@ AC_DEFUN([SVN_CC_MODE_SETUP],
CNOWARNFLAGS="$CFLAGS"
CFLAGS="$CFLAGS_KEEP"
+ CFLAGS="-DSVN__CORE $CFLAGS"
AC_SUBST(CMODEFLAGS)
AC_SUBST(CNOWARNFLAGS)
Index: build/generator/gen_win.py
===================================================================
--- build/generator/gen_win.py (revision 1508471)
+++ build/generator/gen_win.py (working copy)
@@ -714,7 +714,8 @@ class WinGeneratorBase(gen_win_dependencies.GenDep
fakedefines = ["WIN32","_WINDOWS","alloca=_alloca",
"_CRT_SECURE_NO_DEPRECATE=",
"_CRT_NONSTDC_NO_DEPRECATE=",
- "_CRT_SECURE_NO_WARNINGS="]
+ "_CRT_SECURE_NO_WARNINGS=",
+ "SVN__CORE"]
if cfg == 'Debug':
fakedefines.extend(["_DEBUG","SVN_DEBUG"])
Index: subversion/include/svn_hash.h
===================================================================
--- subversion/include/svn_hash.h (revision 1508471)
+++ subversion/include/svn_hash.h (working copy)
@@ -248,7 +248,11 @@ svn_hash_from_cstring_keys(apr_hash_t **hash,
* below will usually generate a compiler warning if your definition is
* being encountered _after_ #including this header.
*/
-#define SVN_HAS_DUNDER_BUILTINS 0
+# ifdef SVN__CORE
+# error "Build broken (should you include svn_private_config.h earlier?)"
+# else
+# define SVN_HAS_DUNDER_BUILTINS 0
+# endif
#endif
/** Shortcut for apr_hash_get() with a const char * key.
Untested, but I'll add it to my queue for next week.
> > 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.
Received on 2013-07-30 16:47:21 CEST