| Message ID | 532B3F64.6030005@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 20 Mar 2014, Adhemerval Zanella wrote: > Ok, I check with gettext implementation > 'gettext-runtime/intl/loadmsgcat.c' and changed the initial patch based > on your suggestion (I also added some changes to make glibc one similar > to gettext one). Any change brought from gettext would best be committed on its own (ideally with the original author of that change in gettext being credited).
On 20-03-2014 21:01, Joseph S. Myers wrote: > On Thu, 20 Mar 2014, Adhemerval Zanella wrote: > >> Ok, I check with gettext implementation >> 'gettext-runtime/intl/loadmsgcat.c' and changed the initial patch based >> on your suggestion (I also added some changes to make glibc one similar >> to gettext one). > Any change brought from gettext would best be committed on its own > (ideally with the original author of that change in gettext being > credited). > The changes I have incorporated on the patch is just change the old K&R function prototype to ISO C, remove and superfluous ';' and change a free call to remove an unnecessary cast. For these straightforward patches do we really need to track the ones how made then and commit a set of patches?
On Thu, 20 Mar 2014, Adhemerval Zanella wrote: > > Any change brought from gettext would best be committed on its own > > (ideally with the original author of that change in gettext being > > credited). > > > The changes I have incorporated on the patch is just change the old K&R > function prototype to ISO C, remove and superfluous ';' and change a > free call to remove an unnecessary cast. For these straightforward patches > do we really need to track the ones how made then and commit a set of > patches? You could probably just say "Change merged from gettext." or similar without detailed attribution - but a change coming from gettext should still be committed separately from a change not coming from gettext.
On 03/20/2014 03:20 PM, Adhemerval Zanella wrote: > diff --git a/intl/loadmsgcat.c b/intl/loadmsgcat.c > index b96a997..d47b903 100644 > --- a/intl/loadmsgcat.c > +++ b/intl/loadmsgcat.c > @@ -62,6 +62,7 @@ char *alloca (); > #ifdef _LIBC > # include <langinfo.h> > # include <locale.h> > +# define PRI_MACROS_BROKEN 0 > #endif > > #if (defined HAVE_MMAP && defined HAVE_MUNMAP && !defined DISALLOW_MMAP) \ This hunk could go in immediately to fix the -Wundef issues while take our time to merge a new gettext update. I was looking at this today, but it's going to be more work than I have free time for this week. Even though Steve Ellcey has gotten upstream to change the code such that PRI_MACROS_BROKEN is always defined, there is no stable release with that change yet. Cheers, Carlos.
On Fri, 2 May 2014, Carlos O'Donell wrote: > Even though Steve Ellcey has gotten upstream to change the code > such that PRI_MACROS_BROKEN is always defined, there is no stable > release with that change yet. I don't think gettext releases are relevant to us; it's the development sources of glibc's libintl that we should aim to have in sync with the development sources of gettext's libintl, by merging local changes in both directions and implementing things in such a way that identical code can work in both places.
On 05/02/2014 11:12 AM, Joseph S. Myers wrote: > On Fri, 2 May 2014, Carlos O'Donell wrote: > >> Even though Steve Ellcey has gotten upstream to change the code >> such that PRI_MACROS_BROKEN is always defined, there is no stable >> release with that change yet. > > I don't think gettext releases are relevant to us; it's the development > sources of glibc's libintl that we should aim to have in sync with the > development sources of gettext's libintl, by merging local changes in both > directions and implementing things in such a way that identical code can > work in both places. May I suggest the following plan of action then? * Merge libintl 0.18.3.2 (official release) into glibc and fixup where appropriate, testing the result. * Pro: You know libitinl works because it's a released version. * Con: Subsequent merge from glibc to gettext is harder. * Review remaining differences between glibc master and gettext master and propose patches. This reduces the merge-in risk of breaking glibc by using a stable gettext, but allows us to iterate down to zero differences. The 0.18.3.2 differences aren't large, but they still have new generic lock thread support, and pathname support, and merging this in worries me slightly. That worry is why I recommend a conservative first step. Are you suggesting that this is really just a waste of time given libintl's stability and doing the above is just 2x the work for no reward? Cheers, Carlos.
On Fri, 2 May 2014, Carlos O'Donell wrote: > The 0.18.3.2 differences aren't large, but they still have > new generic lock thread support, and pathname support, and > merging this in worries me slightly. That worry is why I > recommend a conservative first step. If concerned about any changes, then splitting up the merge makes sense - but splitting it up on the basis of *separate logical changes* rather than using a particular libintl release. (Splitting up matters less for changes that are completely routine cleanups, though such cleanups shouldn't be mixed with substantive changes.)
> I don't think gettext releases are relevant to us; it's the development > sources of glibc's libintl that we should aim to have in sync with the > development sources of gettext's libintl, by merging local changes in both > directions and implementing things in such a way that identical code can > work in both places. Agreed. We don't worry about releasedness for any of the code we share with other GNU projects. We vet the changes like any other changes and rely on our own vetting and our own testing. Of course, for any shared file whose source of truth changes more often than once in a blue moon, it's wise to pay attention to the owning project and make sure we've kept up with all its changes when either that project or libc is nearing release.
On 05/02/2014 03:27 PM, Roland McGrath wrote: >> I don't think gettext releases are relevant to us; it's the development >> sources of glibc's libintl that we should aim to have in sync with the >> development sources of gettext's libintl, by merging local changes in both >> directions and implementing things in such a way that identical code can >> work in both places. > > Agreed. We don't worry about releasedness for any of the code we share > with other GNU projects. We vet the changes like any other changes and > rely on our own vetting and our own testing. Of course, for any shared > file whose source of truth changes more often than once in a blue moon, > it's wise to pay attention to the owning project and make sure we've kept > up with all its changes when either that project or libc is nearing release. I'll look at gettext master then. I appreciate you and Joseph commenting promptly before I wasted my time with an intermediate merge if nobody thought that was a good idea. c.
diff --git a/intl/loadmsgcat.c b/intl/loadmsgcat.c index b96a997..d47b903 100644 --- a/intl/loadmsgcat.c +++ b/intl/loadmsgcat.c @@ -62,6 +62,7 @@ char *alloca (); #ifdef _LIBC # include <langinfo.h> # include <locale.h> +# define PRI_MACROS_BROKEN 0 #endif #if (defined HAVE_MMAP && defined HAVE_MUNMAP && !defined DISALLOW_MMAP) \ @@ -486,8 +487,7 @@ int _nl_msg_cat_cntr; /* Expand a system dependent string segment. Return NULL if unsupported. */ static const char * -get_sysdep_segment_value (name) - const char *name; +get_sysdep_segment_value (const char *name) { /* Test for an ISO C 99 section 7.8.1 format string directive. Syntax: @@ -756,9 +756,8 @@ get_sysdep_segment_value (name) message catalog do nothing. */ void internal_function -_nl_load_domain (domain_file, domainbinding) - struct loaded_l10nfile *domain_file; - struct binding *domainbinding; +_nl_load_domain (struct loaded_l10nfile *domain_file, + struct binding *domainbinding) { __libc_lock_define_initialized_recursive (static, lock); int fd = -1; @@ -821,7 +820,7 @@ _nl_load_domain (domain_file, domainbinding) || __builtin_expect ((size = (size_t) st.st_size) != st.st_size, 0) || __builtin_expect (size < sizeof (struct mo_file_header), 0)) /* Something went wrong. */ - goto out;; + goto out; #ifdef HAVE_MMAP /* Now we are ready to load the file. If mmap() is available we try @@ -1277,8 +1276,7 @@ _nl_load_domain (domain_file, domainbinding) #ifdef _LIBC void internal_function __libc_freeres_fn_section -_nl_unload_domain (domain) - struct loaded_domain *domain; +_nl_unload_domain (struct loaded_domain *domain) { size_t i; @@ -1289,7 +1287,7 @@ _nl_unload_domain (domain) { struct converted_domain *convd = &domain->conversions[i]; - free ((char *) convd->encoding); + free (convd->encoding); if (convd->conv_tab != NULL && convd->conv_tab != (char **) -1) free (convd->conv_tab); if (convd->conv != (__gconv_t) -1)