diff mbox

un-nest findidx()

Message ID CAGQ9bdz9V7w+5Wn_j8CSKvweh9GkW0gU5GvB94UCP2Zn-bf8OQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Kostya Serebryany June 18, 2014, 1:59 p.m. UTC
Hi,

Please review the patch which removes nested functions findidx and replaces them
with two 'static inline' functions: findidx and findidxwc.
No functionality change intended.

un-nesting glibc function was previously discussed here:
https://sourceware.org/ml/libc-alpha/2014-05/msg00400.html

Thanks!

--kcc

2014-06-18  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>

        * locale/weight.h: add include guard.
        (findidx): un-nest, make it static inline, add parameters.
        * locale/weightwc.h: add include guard, rename findidx to findidxwc.
        (findidxwc): un-nest, make it static inline, add parameters.
        * posix/fnmatch_loop.c: include weightwc.h or weight.h depending on
        WIDE_CHAR_VERSION. Define FINDIDX as findidxwc or findidx.
        (FCT): change type of 'extra' to wint_t; do not include weight.h,
        un-nest calls to findidx.
        * posix/regcomp.c: include weight.h.
        (build_equiv_class): don't include weight.h, un-nest findidx.
        * posix/regex_internal.h: include weight.h
        (re_string_elem_size_at): don't include weight.h, un-nest findidx.
        * posix/regexec.c: include weight.h.
        (check_node_accept_bytes): don't include weight.h, un-nest findidx.
        * string/strcoll_l.c: define FINDIDX, include WEIGHT_H.
        (get_next_seq): don't include WEIGHT_H, un-nest findidx.
        (get_next_seq_nocache): don't include WEIGHT_H, un-nest findidx.
        * string/strxfrm_l.c: define FINDIDX, include WEIGHT_H.
        (STRXFRM): don't include WEIGHT_H, un-nest findidx.
        * wcsmbs/wcscoll_l.c: define FINDIDX.
        * wcsmbs/wcsxfrm_l.c: define FINDIDX.

Comments

Kostya Serebryany July 3, 2014, 11:56 a.m. UTC | #1
Any comments on this patch?

--kcc

On Wed, Jun 18, 2014 at 5:59 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> Hi,
>
> Please review the patch which removes nested functions findidx and replaces them
> with two 'static inline' functions: findidx and findidxwc.
> No functionality change intended.
>
> un-nesting glibc function was previously discussed here:
> https://sourceware.org/ml/libc-alpha/2014-05/msg00400.html
>
> Thanks!
>
> --kcc
>
> 2014-06-18  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>
>
>         * locale/weight.h: add include guard.
>         (findidx): un-nest, make it static inline, add parameters.
>         * locale/weightwc.h: add include guard, rename findidx to findidxwc.
>         (findidxwc): un-nest, make it static inline, add parameters.
>         * posix/fnmatch_loop.c: include weightwc.h or weight.h depending on
>         WIDE_CHAR_VERSION. Define FINDIDX as findidxwc or findidx.
>         (FCT): change type of 'extra' to wint_t; do not include weight.h,
>         un-nest calls to findidx.
>         * posix/regcomp.c: include weight.h.
>         (build_equiv_class): don't include weight.h, un-nest findidx.
>         * posix/regex_internal.h: include weight.h
>         (re_string_elem_size_at): don't include weight.h, un-nest findidx.
>         * posix/regexec.c: include weight.h.
>         (check_node_accept_bytes): don't include weight.h, un-nest findidx.
>         * string/strcoll_l.c: define FINDIDX, include WEIGHT_H.
>         (get_next_seq): don't include WEIGHT_H, un-nest findidx.
>         (get_next_seq_nocache): don't include WEIGHT_H, un-nest findidx.
>         * string/strxfrm_l.c: define FINDIDX, include WEIGHT_H.
>         (STRXFRM): don't include WEIGHT_H, un-nest findidx.
>         * wcsmbs/wcscoll_l.c: define FINDIDX.
>         * wcsmbs/wcsxfrm_l.c: define FINDIDX.
Andreas Schwab July 3, 2014, 12:06 p.m. UTC | #2
Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:

>         (FCT): change type of 'extra' to wint_t;

Why do you need this change?

Andreas.
Kostya Serebryany July 3, 2014, 12:20 p.m. UTC | #3
On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>
>>         (FCT): change type of 'extra' to wint_t;
>
> Why do you need this change?
To avoid warnings about different types of argument and parameter
>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Andreas Schwab July 3, 2014, 12:26 p.m. UTC | #4
Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:

> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>
>>>         (FCT): change type of 'extra' to wint_t;
>>
>> Why do you need this change?
> To avoid warnings about different types of argument and parameter

But why do you have that conflict in the first place?  You must use the
type of the object in the end.

Andreas.
Kostya Serebryany July 3, 2014, 1:01 p.m. UTC | #5
On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab <schwab@suse.de> wrote:
> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>
>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>
>>>>         (FCT): change type of 'extra' to wint_t;
>>>
>>> Why do you need this change?
>> To avoid warnings about different types of argument and parameter
>
> But why do you have that conflict in the first place?  You must use the
> type of the object in the end.

There are multiple uses of findidxwc. Some were using int32_t* for
'extra', some where using wint_t*
(e.g. see USTRING_TYPE in wcsmbs/wcscoll.c).
When un-nesting findidxwc we have to chose just one type. I chose
wint_t because otherwise the change would have been larger.

--kcc

>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Kostya Serebryany July 25, 2014, 10:05 a.m. UTC | #6
Any other comments?

On Thu, Jul 3, 2014 at 5:01 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab <schwab@suse.de> wrote:
>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>
>>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>
>>>>>         (FCT): change type of 'extra' to wint_t;
>>>>
>>>> Why do you need this change?
>>> To avoid warnings about different types of argument and parameter
>>
>> But why do you have that conflict in the first place?  You must use the
>> type of the object in the end.
>
> There are multiple uses of findidxwc. Some were using int32_t* for
> 'extra', some where using wint_t*
> (e.g. see USTRING_TYPE in wcsmbs/wcscoll.c).
> When un-nesting findidxwc we have to chose just one type. I chose
> wint_t because otherwise the change would have been larger.
>
> --kcc
>
>>
>> Andreas.
>>
>> --
>> Andreas Schwab, SUSE Labs, schwab@suse.de
>> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
>> "And now for something completely different."
Will Newton July 25, 2014, 11:48 a.m. UTC | #7
On 25 July 2014 11:05, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> Any other comments?

The comments on the include guards are a little odd, usually the name
of the define is used rather than the filename. Besides that minor nit
the code looks ok to me and also passes make check. I don't think it
is something we would want to take at this stage in the freeze
however.

> On Thu, Jul 3, 2014 at 5:01 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab <schwab@suse.de> wrote:
>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>
>>>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>>
>>>>>>         (FCT): change type of 'extra' to wint_t;
>>>>>
>>>>> Why do you need this change?
>>>> To avoid warnings about different types of argument and parameter
>>>
>>> But why do you have that conflict in the first place?  You must use the
>>> type of the object in the end.
>>
>> There are multiple uses of findidxwc. Some were using int32_t* for
>> 'extra', some where using wint_t*
>> (e.g. see USTRING_TYPE in wcsmbs/wcscoll.c).
>> When un-nesting findidxwc we have to chose just one type. I chose
>> wint_t because otherwise the change would have been larger.
>>
>> --kcc
>>
>>>
>>> Andreas.
>>>
>>> --
>>> Andreas Schwab, SUSE Labs, schwab@suse.de
>>> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
>>> "And now for something completely different."
Kostya Serebryany July 25, 2014, 1:12 p.m. UTC | #8
On Fri, Jul 25, 2014 at 3:48 PM, Will Newton <will.newton@linaro.org> wrote:
> On 25 July 2014 11:05, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> Any other comments?
>
> The comments on the include guards are a little odd,
You mean the ChangeLog entry?

Should it be like this?
2014-07-25  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>

        * locale/weight.h (_WEIGHT_H_): add include guard.
        (findidx): un-nest, make it static inline, add parameters.
        * locale/weightwc.h (_WEIGHTWC_H_): add include guard.
        (findidx): rename to findidxwc, un-nest, make it static inline,
        add parameters.
        * posix/fnmatch_loop.c: include weightwc.h or weight.h depending on
        WIDE_CHAR_VERSION. Define FINDIDX as findidxwc or findidx.
        (FCT): change type of 'extra' to wint_t; do not include weight.h,
        un-nest calls to findidx.
        * posix/regcomp.c: include weight.h.
        (build_equiv_class): don't include weight.h, un-nest findidx.
        * posix/regex_internal.h: include weight.h
        (re_string_elem_size_at): don't include weight.h, un-nest findidx.
        * posix/regexec.c: include weight.h.
        (check_node_accept_bytes): don't include weight.h, un-nest findidx.
        * string/strcoll_l.c: define FINDIDX, include WEIGHT_H.
        (get_next_seq): don't include WEIGHT_H, un-nest findidx.
        (get_next_seq_nocache): don't include WEIGHT_H, un-nest findidx.
        * string/strxfrm_l.c: define FINDIDX, include WEIGHT_H.
        (STRXFRM): don't include WEIGHT_H, un-nest findidx.
        * wcsmbs/wcscoll_l.c: define FINDIDX.
        * wcsmbs/wcsxfrm_l.c: define FINDIDX.



> usually the name
> of the define is used rather than the filename. Besides that minor nit
> the code looks ok to me and also passes make check. I don't think it
> is something we would want to take at this stage in the freeze
> however.

Ok, let me ping this thread after the freeze is over. Thanks!

--kcc


>
>> On Thu, Jul 3, 2014 at 5:01 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>
>>>>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>>>
>>>>>>>         (FCT): change type of 'extra' to wint_t;
>>>>>>
>>>>>> Why do you need this change?
>>>>> To avoid warnings about different types of argument and parameter
>>>>
>>>> But why do you have that conflict in the first place?  You must use the
>>>> type of the object in the end.
>>>
>>> There are multiple uses of findidxwc. Some were using int32_t* for
>>> 'extra', some where using wint_t*
>>> (e.g. see USTRING_TYPE in wcsmbs/wcscoll.c).
>>> When un-nesting findidxwc we have to chose just one type. I chose
>>> wint_t because otherwise the change would have been larger.
>>>
>>> --kcc
>>>
>>>>
>>>> Andreas.
>>>>
>>>> --
>>>> Andreas Schwab, SUSE Labs, schwab@suse.de
>>>> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
>>>> "And now for something completely different."
>
>
>
> --
> Will Newton
> Toolchain Working Group, Linaro
Will Newton July 25, 2014, 2:29 p.m. UTC | #9
On 25 July 2014 14:12, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Fri, Jul 25, 2014 at 3:48 PM, Will Newton <will.newton@linaro.org> wrote:
>> On 25 July 2014 11:05, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> Any other comments?
>>
>> The comments on the include guards are a little odd,
> You mean the ChangeLog entry?

The ChangeLog looks ok (although sentences should start with capitals)
but I was referring to the comments on the include guards:

@ -130,3 +136,5 @@ findidx (const unsigned char **cpp, size_t len)
   /* NOTREACHED */
   return 0x43219876;
 }
+
+#endif  /* weight.h */

Which I would usually expect to see as something like:

#endif /* !_WEIGHT_H_ */

But it's a small nit indeed...

> Should it be like this?
> 2014-07-25  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>
>
>         * locale/weight.h (_WEIGHT_H_): add include guard.
>         (findidx): un-nest, make it static inline, add parameters.
>         * locale/weightwc.h (_WEIGHTWC_H_): add include guard.
>         (findidx): rename to findidxwc, un-nest, make it static inline,
>         add parameters.
>         * posix/fnmatch_loop.c: include weightwc.h or weight.h depending on
>         WIDE_CHAR_VERSION. Define FINDIDX as findidxwc or findidx.
>         (FCT): change type of 'extra' to wint_t; do not include weight.h,
>         un-nest calls to findidx.
>         * posix/regcomp.c: include weight.h.
>         (build_equiv_class): don't include weight.h, un-nest findidx.
>         * posix/regex_internal.h: include weight.h
>         (re_string_elem_size_at): don't include weight.h, un-nest findidx.
>         * posix/regexec.c: include weight.h.
>         (check_node_accept_bytes): don't include weight.h, un-nest findidx.
>         * string/strcoll_l.c: define FINDIDX, include WEIGHT_H.
>         (get_next_seq): don't include WEIGHT_H, un-nest findidx.
>         (get_next_seq_nocache): don't include WEIGHT_H, un-nest findidx.
>         * string/strxfrm_l.c: define FINDIDX, include WEIGHT_H.
>         (STRXFRM): don't include WEIGHT_H, un-nest findidx.
>         * wcsmbs/wcscoll_l.c: define FINDIDX.
>         * wcsmbs/wcsxfrm_l.c: define FINDIDX.
>
>
>
>> usually the name
>> of the define is used rather than the filename. Besides that minor nit
>> the code looks ok to me and also passes make check. I don't think it
>> is something we would want to take at this stage in the freeze
>> however.
>
> Ok, let me ping this thread after the freeze is over. Thanks!
>
> --kcc
>
>
>>
>>> On Thu, Jul 3, 2014 at 5:01 PM, Konstantin Serebryany
>>> <konstantin.s.serebryany@gmail.com> wrote:
>>>> On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>>
>>>>>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>>>>
>>>>>>>>         (FCT): change type of 'extra' to wint_t;
>>>>>>>
>>>>>>> Why do you need this change?
>>>>>> To avoid warnings about different types of argument and parameter
>>>>>
>>>>> But why do you have that conflict in the first place?  You must use the
>>>>> type of the object in the end.
>>>>
>>>> There are multiple uses of findidxwc. Some were using int32_t* for
>>>> 'extra', some where using wint_t*
>>>> (e.g. see USTRING_TYPE in wcsmbs/wcscoll.c).
>>>> When un-nesting findidxwc we have to chose just one type. I chose
>>>> wint_t because otherwise the change would have been larger.
>>>>
>>>> --kcc
>>>>
>>>>>
>>>>> Andreas.
>>>>>
>>>>> --
>>>>> Andreas Schwab, SUSE Labs, schwab@suse.de
>>>>> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
>>>>> "And now for something completely different."
>>
>>
>>
>> --
>> Will Newton
>> Toolchain Working Group, Linaro
Carlos O'Donell July 25, 2014, 6:21 p.m. UTC | #10
On 07/25/2014 10:29 AM, Will Newton wrote:
> +#endif  /* weight.h */
> 
> Which I would usually expect to see as something like:
> 
> #endif /* !_WEIGHT_H_ */
> 
> But it's a small nit indeed...

I would immediately clean this up to !_WEIGHT_H_ if I saw
it in code, which means it's a part of our coding style and
should be adjusted to match existing style.

I like it because it immediately alerts you to the condition
that started the conditional.

Cheers,
Carlos.
Kostya Serebryany July 25, 2014, 7:19 p.m. UTC | #11
I certainly don't care which way to choose, but a quick grep shows
that both  styles are frequent:

% grep  '#endif.*/\*.*[A-Z]' */*.h
argp/argp-fmtstream.h:#endif /* __OPTIMIZE__ */
argp/argp-fmtstream.h:#endif /* ARGP_FMTSTREAM_USE_LINEWRAP */
argp/argp.h:#endif /* Use extern inlines.  */
argp/argp-namefrob.h:#endif /* !_LIBC */
assert/assert.h:#endif /* NDEBUG.  */
benchtests/bench-string.h:#endif /* TEST_MAIN */
bits/byteswap.h:#endif /* _BITS_BYTESWAP_H */
bits/environments.h:#endif /* __WORDSIZE == 32 */
bits/mathdef.h:#endif   /* ISO C99 */
bits/siginfo.h:#endif   /* !have siginfo_t && (have _SIGNAL_H || need
siginfo_t).  */
bits/siginfo.h:#endif   /* have _SIGNAL_H.  */
bits/sigset.h:#endif /* ! _SIGSET_H_fns.  */
...
248 total

%  grep '#endif.*/\*.*\.h' */*.h
argp/argp-fmtstream.h:#endif /* argp-fmtstream.h */
argp/argp.h:#endif /* argp.h */
assert/assert.h:#endif /* assert.h      */
b/abi-versions.h:#endif /* abi-versions.h */
bits/atomic.h:#endif    /* bits/atomic.h */
bits/ipctypes.h:#endif /* bits/ipctypes.h */
bits/libc-lock.h:#endif /* bits/libc-lock.h */
bits/libc-tsd.h:#endif  /* bits/libc-tsd.h */
bits/signum.h:#endif    /* <signal.h> included.  */
bits/sigthread.h:#endif /* bits/sigthread.h */
bits/sockaddr.h:#endif  /* bits/sockaddr.h */
bits/socket.h:#endif    /* bits/socket.h */
184 total


--kcc

On Fri, Jul 25, 2014 at 10:21 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 07/25/2014 10:29 AM, Will Newton wrote:
>> +#endif  /* weight.h */
>>
>> Which I would usually expect to see as something like:
>>
>> #endif /* !_WEIGHT_H_ */
>>
>> But it's a small nit indeed...
>
> I would immediately clean this up to !_WEIGHT_H_ if I saw
> it in code, which means it's a part of our coding style and
> should be adjusted to match existing style.
>
> I like it because it immediately alerts you to the condition
> that started the conditional.
>
> Cheers,
> Carlos.
Carlos O'Donell July 25, 2014, 8:37 p.m. UTC | #12
On 07/25/2014 03:19 PM, Konstantin Serebryany wrote:
> I certainly don't care which way to choose, but a quick grep shows
> that both  styles are frequent:

This isn't quite an apples-to-apples comparison.

For commenting conditionals I think it's overwhelmingly
the case that the closing #endif contains the conditional
that was true for that branch.

The case of conditionals that are file inclusion guards
is unique though, and it turns out I'm wrong here. In this
case the overwhelming majority close out the conditional
with the name of the file. This makes sense since it gives
the most information to the developer that it is (a) an
inclusion guard and (b) which file. The down side being
that if the file is moved you need to update the inclusion
guard comment.

I think both are acceptable and I've commented as such in
the Style and Conventions guide.

https://sourceware.org/glibc/wiki/Style_and_Conventions#Commenting_.23endif

I retract my statement that I'd change your patch.

Cheers,
Carlos.
Kostya Serebryany Sept. 8, 2014, 6:08 p.m. UTC | #13
2.20 has been released.
Can we now proceed with this patch?
(I've double-checked, it still applies, builds and validates)

Thanks,

--kcc

On Fri, Jul 25, 2014 at 6:12 AM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Fri, Jul 25, 2014 at 3:48 PM, Will Newton <will.newton@linaro.org> wrote:
>> On 25 July 2014 11:05, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> Any other comments?
>>
>> The comments on the include guards are a little odd,
> You mean the ChangeLog entry?
>
> Should it be like this?
> 2014-07-25  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>
>
>         * locale/weight.h (_WEIGHT_H_): add include guard.
>         (findidx): un-nest, make it static inline, add parameters.
>         * locale/weightwc.h (_WEIGHTWC_H_): add include guard.
>         (findidx): rename to findidxwc, un-nest, make it static inline,
>         add parameters.
>         * posix/fnmatch_loop.c: include weightwc.h or weight.h depending on
>         WIDE_CHAR_VERSION. Define FINDIDX as findidxwc or findidx.
>         (FCT): change type of 'extra' to wint_t; do not include weight.h,
>         un-nest calls to findidx.
>         * posix/regcomp.c: include weight.h.
>         (build_equiv_class): don't include weight.h, un-nest findidx.
>         * posix/regex_internal.h: include weight.h
>         (re_string_elem_size_at): don't include weight.h, un-nest findidx.
>         * posix/regexec.c: include weight.h.
>         (check_node_accept_bytes): don't include weight.h, un-nest findidx.
>         * string/strcoll_l.c: define FINDIDX, include WEIGHT_H.
>         (get_next_seq): don't include WEIGHT_H, un-nest findidx.
>         (get_next_seq_nocache): don't include WEIGHT_H, un-nest findidx.
>         * string/strxfrm_l.c: define FINDIDX, include WEIGHT_H.
>         (STRXFRM): don't include WEIGHT_H, un-nest findidx.
>         * wcsmbs/wcscoll_l.c: define FINDIDX.
>         * wcsmbs/wcsxfrm_l.c: define FINDIDX.
>
>
>
>> usually the name
>> of the define is used rather than the filename. Besides that minor nit
>> the code looks ok to me and also passes make check. I don't think it
>> is something we would want to take at this stage in the freeze
>> however.
>
> Ok, let me ping this thread after the freeze is over. Thanks!
>
> --kcc
>
>
>>
>>> On Thu, Jul 3, 2014 at 5:01 PM, Konstantin Serebryany
>>> <konstantin.s.serebryany@gmail.com> wrote:
>>>> On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>>
>>>>>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab <schwab@suse.de> wrote:
>>>>>>> Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
>>>>>>>
>>>>>>>>         (FCT): change type of 'extra' to wint_t;
>>>>>>>
>>>>>>> Why do you need this change?
>>>>>> To avoid warnings about different types of argument and parameter
>>>>>
>>>>> But why do you have that conflict in the first place?  You must use the
>>>>> type of the object in the end.
>>>>
>>>> There are multiple uses of findidxwc. Some were using int32_t* for
>>>> 'extra', some where using wint_t*
>>>> (e.g. see USTRING_TYPE in wcsmbs/wcscoll.c).
>>>> When un-nesting findidxwc we have to chose just one type. I chose
>>>> wint_t because otherwise the change would have been larger.
>>>>
>>>> --kcc
>>>>
>>>>>
>>>>> Andreas.
>>>>>
>>>>> --
>>>>> Andreas Schwab, SUSE Labs, schwab@suse.de
>>>>> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
>>>>> "And now for something completely different."
>>
>>
>>
>> --
>> Will Newton
>> Toolchain Working Group, Linaro
Roland McGrath Sept. 9, 2014, 9:57 p.m. UTC | #14
When it's been very long at all, you should post a fresh patch that's
verified against today's trunk, and not include any quoted nonsense in the
message.  Just post in the same thread and that should be enough context
for everybody.
diff mbox

Patch

diff --git a/locale/weight.h b/locale/weight.h
index 9eb8ac6..485526c 100644
--- a/locale/weight.h
+++ b/locale/weight.h
@@ -16,10 +16,16 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#ifndef _WEIGHT_H_
+#define _WEIGHT_H_
+
 /* Find index of weight.  */
-auto inline int32_t
+static inline int32_t
 __attribute ((always_inline))
-findidx (const unsigned char **cpp, size_t len)
+findidx (const int32_t *table,
+	 const int32_t *indirect,
+	 const unsigned char *extra,
+	 const unsigned char **cpp, size_t len)
 {
   int_fast32_t i = table[*(*cpp)++];
   const unsigned char *cp;
@@ -130,3 +136,5 @@  findidx (const unsigned char **cpp, size_t len)
   /* NOTREACHED */
   return 0x43219876;
 }
+
+#endif  /* weight.h */
diff --git a/locale/weightwc.h b/locale/weightwc.h
index 8f047e3..3348544 100644
--- a/locale/weightwc.h
+++ b/locale/weightwc.h
@@ -16,10 +16,16 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#ifndef _WEIGHTWC_H_
+#define _WEIGHTWC_H_
+
 /* Find index of weight.  */
-auto inline int32_t
+static inline int32_t
 __attribute ((always_inline))
-findidx (const wint_t **cpp, size_t len)
+findidxwc (const int32_t *table,
+	   const int32_t *indirect,
+	   const wint_t *extra,
+	   const wint_t **cpp, size_t len)
 {
   wint_t ch = *(*cpp)++;
   int32_t i = __collidx_table_lookup ((const char *) table, ch);
@@ -109,3 +115,5 @@  findidx (const wint_t **cpp, size_t len)
   /* NOTREACHED */
   return 0x43219876;
 }
+
+#endif  /* weightwc.h */
diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
index f79d051..8cd2f31 100644
--- a/posix/fnmatch_loop.c
+++ b/posix/fnmatch_loop.c
@@ -17,6 +17,17 @@ 
 
 #include <stdint.h>
 
+# if WIDE_CHAR_VERSION
+#  include <locale/weightwc.h>
+#  undef FINDIDX
+#  define FINDIDX findidxwc
+# else
+#  include <locale/weight.h>
+#  undef FINDIDX
+#  define FINDIDX findidx
+# endif
+
+
 struct STRUCT
 {
   const CHAR *pattern;
@@ -376,7 +387,7 @@  FCT (pattern, string, string_end, no_leading_period, flags, ends, alloca_used)
 			const int32_t *table;
 # if WIDE_CHAR_VERSION
 			const int32_t *weights;
-			const int32_t *extra;
+			const wint_t *extra;
 # else
 			const unsigned char *weights;
 			const unsigned char *extra;
@@ -385,19 +396,12 @@  FCT (pattern, string, string_end, no_leading_period, flags, ends, alloca_used)
 			int32_t idx;
 			const UCHAR *cp = (const UCHAR *) str;
 
-			/* This #include defines a local function!  */
-# if WIDE_CHAR_VERSION
-#  include <locale/weightwc.h>
-# else
-#  include <locale/weight.h>
-# endif
-
 # if WIDE_CHAR_VERSION
 			table = (const int32_t *)
 			  _NL_CURRENT (LC_COLLATE, _NL_COLLATE_TABLEWC);
 			weights = (const int32_t *)
 			  _NL_CURRENT (LC_COLLATE, _NL_COLLATE_WEIGHTWC);
-			extra = (const int32_t *)
+			extra = (const wint_t *)
 			  _NL_CURRENT (LC_COLLATE, _NL_COLLATE_EXTRAWC);
 			indirect = (const int32_t *)
 			  _NL_CURRENT (LC_COLLATE, _NL_COLLATE_INDIRECTWC);
@@ -412,7 +416,7 @@  FCT (pattern, string, string_end, no_leading_period, flags, ends, alloca_used)
 			  _NL_CURRENT (LC_COLLATE, _NL_COLLATE_INDIRECTMB);
 # endif
 
-			idx = findidx (&cp, 1);
+			idx = FINDIDX (table, indirect, extra, &cp, 1);
 			if (idx != 0)
 			  {
 			    /* We found a table entry.  Now see whether the
@@ -422,7 +426,8 @@  FCT (pattern, string, string_end, no_leading_period, flags, ends, alloca_used)
 			    int32_t idx2;
 			    const UCHAR *np = (const UCHAR *) n;
 
-			    idx2 = findidx (&np, string_end - n);
+			    idx2 = FINDIDX (table, indirect, extra,
+					    &np, string_end - n);
 			    if (idx2 != 0
 				&& (idx >> 24) == (idx2 >> 24)
 				&& len == weights[idx2 & 0xffffff])
diff --git a/posix/regcomp.c b/posix/regcomp.c
index 921d0f4..b1cc8bc 100644
--- a/posix/regcomp.c
+++ b/posix/regcomp.c
@@ -3389,6 +3389,8 @@  parse_bracket_symbol (bracket_elem_t *elem, re_string_t *regexp,
   return REG_NOERROR;
 }
 
+#include <locale/weight.h>
+
   /* Helper function for parse_bracket_exp.
      Build the equivalence class which is represented by NAME.
      The result are written to MBCSET and SBCSET.
@@ -3413,8 +3415,6 @@  build_equiv_class (bitset_t sbcset, const unsigned char *name)
       int32_t idx1, idx2;
       unsigned int ch;
       size_t len;
-      /* This #include defines a local function!  */
-# include <locale/weight.h>
       /* Calculate the index for equivalence class.  */
       cp = name;
       table = (const int32_t *) _NL_CURRENT (LC_COLLATE, _NL_COLLATE_TABLEMB);
@@ -3424,7 +3424,7 @@  build_equiv_class (bitset_t sbcset, const unsigned char *name)
 						   _NL_COLLATE_EXTRAMB);
       indirect = (const int32_t *) _NL_CURRENT (LC_COLLATE,
 						_NL_COLLATE_INDIRECTMB);
-      idx1 = findidx (&cp, -1);
+      idx1 = findidx (table, indirect, extra, &cp, -1);
       if (BE (idx1 == 0 || *cp != '\0', 0))
 	/* This isn't a valid character.  */
 	return REG_ECOLLATE;
@@ -3435,7 +3435,7 @@  build_equiv_class (bitset_t sbcset, const unsigned char *name)
 	{
 	  char_buf[0] = ch;
 	  cp = char_buf;
-	  idx2 = findidx (&cp, 1);
+	  idx2 = findidx (table, indirect, extra, &cp, 1);
 /*
 	  idx2 = table[ch];
 */
diff --git a/posix/regex_internal.h b/posix/regex_internal.h
index 75c390f..c38980b 100644
--- a/posix/regex_internal.h
+++ b/posix/regex_internal.h
@@ -733,6 +733,8 @@  re_string_wchar_at (const re_string_t *pstr, int idx)
 }
 
 # ifndef NOT_IN_libc
+# include <locale/weight.h>
+
 static int
 internal_function __attribute__ ((pure, unused))
 re_string_elem_size_at (const re_string_t *pstr, int idx)
@@ -740,7 +742,6 @@  re_string_elem_size_at (const re_string_t *pstr, int idx)
 #  ifdef _LIBC
   const unsigned char *p, *extra;
   const int32_t *table, *indirect;
-#   include <locale/weight.h>
   uint_fast32_t nrules = _NL_CURRENT_WORD (LC_COLLATE, _NL_COLLATE_NRULES);
 
   if (nrules != 0)
@@ -751,7 +752,7 @@  re_string_elem_size_at (const re_string_t *pstr, int idx)
       indirect = (const int32_t *) _NL_CURRENT (LC_COLLATE,
 						_NL_COLLATE_INDIRECTMB);
       p = pstr->mbs + idx;
-      findidx (&p, pstr->len - idx);
+      findidx (table, indirect, extra, &p, pstr->len - idx);
       return p - pstr->mbs - idx;
     }
   else
diff --git a/posix/regexec.c b/posix/regexec.c
index 7032da7..3e52682 100644
--- a/posix/regexec.c
+++ b/posix/regexec.c
@@ -3749,6 +3749,8 @@  group_nodes_into_DFAstates (const re_dfa_t *dfa, const re_dfastate_t *state,
    one collating element like '.', '[a-z]', opposite to the other nodes
    can only accept one byte.  */
 
+#include <locale/weight.h>
+
 static int
 internal_function
 check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
@@ -3868,8 +3870,6 @@  check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
 	  const int32_t *table, *indirect;
 	  const unsigned char *weights, *extra;
 	  const char *collseqwc;
-	  /* This #include defines a local function!  */
-#  include <locale/weight.h>
 
 	  /* match with collating_symbol?  */
 	  if (cset->ncoll_syms)
@@ -3925,7 +3925,7 @@  check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
 		_NL_CURRENT (LC_COLLATE, _NL_COLLATE_EXTRAMB);
 	      indirect = (const int32_t *)
 		_NL_CURRENT (LC_COLLATE, _NL_COLLATE_INDIRECTMB);
-	      int32_t idx = findidx (&cp, elem_len);
+	      int32_t idx = findidx (table, indirect, extra, &cp, elem_len);
 	      if (idx > 0)
 		for (i = 0; i < cset->nequiv_classes; ++i)
 		  {
diff --git a/string/strcoll_l.c b/string/strcoll_l.c
index 10ce4a6..8764e67 100644
--- a/string/strcoll_l.c
+++ b/string/strcoll_l.c
@@ -33,6 +33,7 @@ 
 # define STRCMP strcmp
 # define STRLEN strlen
 # define WEIGHT_H "../locale/weight.h"
+# define FINDIDX findidx
 # define SUFFIX	MB
 # define L(arg) arg
 #endif
@@ -146,13 +147,14 @@  get_next_seq_cached (coll_seq *seq, int nrules, int pass,
   seq->idxnow = idxnow;
 }
 
+#include WEIGHT_H
+
 /* Get next sequence.  Traverse the string as required.  */
 static void
 get_next_seq (coll_seq *seq, int nrules, const unsigned char *rulesets,
 	      const USTRING_TYPE *weights, const int32_t *table,
 	      const USTRING_TYPE *extra, const int32_t *indirect)
 {
-#include WEIGHT_H
   size_t val = seq->val = 0;
   int len = seq->len;
   size_t backw_stop = seq->backw_stop;
@@ -194,7 +196,7 @@  get_next_seq (coll_seq *seq, int nrules, const unsigned char *rulesets,
 
 	  while (*us != L('\0'))
 	    {
-	      int32_t tmp = findidx (&us, -1);
+	      int32_t tmp = FINDIDX (table, indirect, extra, &us, -1);
 	      rulearr[idxmax] = tmp >> 24;
 	      idxarr[idxmax] = tmp & 0xffffff;
 	      idxcnt = idxmax++;
@@ -242,7 +244,6 @@  get_next_seq_nocache (coll_seq *seq, int nrules, const unsigned char *rulesets,
 		      const USTRING_TYPE *extra, const int32_t *indirect,
 		      int pass)
 {
-#include WEIGHT_H
   size_t val = seq->val = 0;
   int len = seq->len;
   size_t backw_stop = seq->backw_stop;
@@ -285,7 +286,7 @@  get_next_seq_nocache (coll_seq *seq, int nrules, const unsigned char *rulesets,
 	      us = seq->back_us;
 	      while (i < backw)
 		{
-		  int32_t tmp = findidx (&us, -1);
+		  int32_t tmp = FINDIDX (table, indirect, extra, &us, -1);
 		  idx = tmp & 0xffffff;
 		  i++;
 		}
@@ -300,7 +301,7 @@  get_next_seq_nocache (coll_seq *seq, int nrules, const unsigned char *rulesets,
 
 	  while (*us != L('\0'))
 	    {
-	      int32_t tmp = findidx (&us, -1);
+	      int32_t tmp = FINDIDX (table, indirect, extra, &us, -1);
 	      unsigned char rule = tmp >> 24;
 	      prev_idx = idx;
 	      idx = tmp & 0xffffff;
diff --git a/string/strxfrm_l.c b/string/strxfrm_l.c
index 04b9338..7d41a28 100644
--- a/string/strxfrm_l.c
+++ b/string/strxfrm_l.c
@@ -33,6 +33,7 @@ 
 # define STRLEN strlen
 # define STPNCPY __stpncpy
 # define WEIGHT_H "../locale/weight.h"
+# define FINDIDX findidx
 # define SUFFIX	MB
 # define L(arg) arg
 #endif
@@ -80,6 +81,7 @@  utf8_encode (char *buf, int val)
 }
 #endif
 
+#include WEIGHT_H
 
 size_t
 STRXFRM (STRING_TYPE *dest, const STRING_TYPE *src, size_t n, __locale_t l)
@@ -104,8 +106,6 @@  STRXFRM (STRING_TYPE *dest, const STRING_TYPE *src, size_t n, __locale_t l)
   size_t idxcnt;
   int use_malloc;
 
-#include WEIGHT_H
-
   if (nrules == 0)
     {
       if (n != 0)
@@ -174,7 +174,7 @@  STRXFRM (STRING_TYPE *dest, const STRING_TYPE *src, size_t n, __locale_t l)
   idxmax = 0;
   do
     {
-      int32_t tmp = findidx (&usrc, -1);
+      int32_t tmp = FINDIDX (table, indirect, extra, &usrc, -1);
       rulearr[idxmax] = tmp >> 24;
       idxarr[idxmax] = tmp & 0xffffff;
 
diff --git a/wcsmbs/wcscoll_l.c b/wcsmbs/wcscoll_l.c
index 74e2e39..a402806 100644
--- a/wcsmbs/wcscoll_l.c
+++ b/wcsmbs/wcscoll_l.c
@@ -26,6 +26,7 @@ 
 #define STRCMP wcscmp
 #define STRLEN __wcslen
 #define WEIGHT_H "../locale/weightwc.h"
+#define FINDIDX findidxwc
 #define SUFFIX	WC
 #define L(arg) L##arg
 #define WIDE_CHAR_VERSION 1
diff --git a/wcsmbs/wcsxfrm_l.c b/wcsmbs/wcsxfrm_l.c
index f3f3f50..23e1703 100644
--- a/wcsmbs/wcsxfrm_l.c
+++ b/wcsmbs/wcsxfrm_l.c
@@ -26,6 +26,7 @@ 
 #define STRLEN __wcslen
 #define STPNCPY __wcpncpy
 #define WEIGHT_H "../locale/weightwc.h"
+#define FINDIDX findidxwc
 #define SUFFIX  WC
 #define L(arg) L##arg
 #define WIDE_CHAR_VERSION 1