Commit Message
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
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.
Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:
> (FCT): change type of 'extra' to wint_t;
Why do you need this change?
Andreas.
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."
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.
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."
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."
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."
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
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
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.
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.
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.
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
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.
@@ -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 */
@@ -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 */
@@ -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])
@@ -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];
*/
@@ -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
@@ -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)
{
@@ -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;
@@ -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;
@@ -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
@@ -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