From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1497 invoked by alias); 25 Jul 2014 14:29:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 1483 invoked by uid 89); 25 Jul 2014 14:29:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ig0-f171.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=hfsVwPCzqgEBUTRin3QJCML8K+nIXwn15Dihntn6g70=; b=mJxK/5vEHs6K3LJwbxwSB6A3BoeFGyPeXhNcu0ix1HE009Rr66JL27RxjyC0JdIB2e veLEH0uL9kkUvH+DBLFvRZ26hwPY2T/wVef1laJ2z+uZmh4sjGgtwJRQUOG8G+b+eYBb gDm7+sYIK/D6evg4ewtvYrQCDUlGRbEQ2pgg3w8NCwCIoQY/UnIMQgoKCk3J2VYiVTEv E+Jjs3yyR3YJOEUoYtnhcHbJUV+FtJMkH4CLNOoOrvp7Kugmo0Unt7aM7D4dYfO7j88h 8Acn7lC1CwzYCNWpbRbDy6CFDgkDwxQ08DEfhFdinjyXeUrkHtnu+lvkrMONrlGN/rS4 0ZzQ== X-Gm-Message-State: ALoCoQn9oWaHoHGIlgmbj1kRa0niHAnOg1u4E/nPf9cO0iJUAdGE4cFae0NEfvamubZWAKd1mCM/ MIME-Version: 1.0 X-Received: by 10.42.214.207 with SMTP id hb15mr21315378icb.30.1406298559727; Fri, 25 Jul 2014 07:29:19 -0700 (PDT) In-Reply-To: References: Date: Fri, 25 Jul 2014 14:29:00 -0000 Message-ID: Subject: Re: [PATCH] un-nest findidx() From: Will Newton To: Konstantin Serebryany Cc: Andreas Schwab , Roland McGrath , GNU C Library Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2014-07/txt/msg00496.txt.bz2 On 25 July 2014 14:12, Konstantin Serebryany wrote: > On Fri, Jul 25, 2014 at 3:48 PM, Will Newton wrote: >> On 25 July 2014 11:05, Konstantin Serebryany >> 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 > > * 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 >>> wrote: >>>> On Thu, Jul 3, 2014 at 4:26 PM, Andreas Schwab wrote: >>>>> Konstantin Serebryany writes: >>>>> >>>>>> On Thu, Jul 3, 2014 at 4:06 PM, Andreas Schwab wrote: >>>>>>> Konstantin Serebryany 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 Toolchain Working Group, Linaro