Message ID | 0e2a0dfe-5a44-f365-9544-ee0fc2b0df63@linaro.org |
---|---|
State | Dropped |
Headers |
Received: (qmail 102329 invoked by alias); 19 Sep 2017 17:35:10 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 102299 invoked by uid 89); 19 Sep 2017 17:35:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f173.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=dcBEgmvtQ6bqB96+W0EGI3N+Nkft72kTr4TOSNjwwRE=; b=nKZqn1CWQV3G4asgcsChK2gz39YqcpwADXolq/5AGwlzvhlxhPdOcCXPvOnmnz51fM 1MBMBMiSuv2D3fYadr5Wj2L5CP/ERIp2Mh97ECK7RYl/A1exM0fty15kbiYoYQdDP35/ jtpb9qv+f38tlQwDQojPuIqKzS9VDV9NyhIPXiicuVQ1dkAiSgloFrUzcJaeq+Hu6rh3 PL5oK4qE8n/Gh/+v2tfydE+3r6AqbaHCQcN/H0JzMkYMn+Sx8HjzKYYp+Eh3qTzuFSXL s791N62L7YYJtT0TVHMCqtPdBKqPNrV1rR2x8iiOm7dgmf4W57Ptg+rI5J0gaDLCkKHK Zwfw== X-Gm-Message-State: AHPjjUiyE2lSqvhsPsJuNwYqLg0OgDg9osDbTCn24dalAhO6z8QccFpp mCy88JvXB1UeM1uavkqLEFVIIVzWwQg= X-Google-Smtp-Source: AOwi7QCs6x6vk7HZmdR6bPLwVo64QcHgW0AhgS6Tj3eibZFuboQbDlVDiyCydFysBMqvxi+wOKoK7Q== X-Received: by 10.200.8.131 with SMTP id v3mr3110008qth.262.1505842504830; Tue, 19 Sep 2017 10:35:04 -0700 (PDT) Subject: Re: [PATCH 2/2] Use C11 _Alignas on scratch_buffer internal buffer To: Andreas Schwab <schwab@suse.de> Cc: Paul Eggert <eggert@cs.ucla.edu>, Florian Weimer <fweimer@redhat.com>, libc-alpha@sourceware.org References: <1505745774-29303-1-git-send-email-adhemerval.zanella@linaro.org> <1505745774-29303-2-git-send-email-adhemerval.zanella@linaro.org> <0f162389-e84d-3168-35eb-f168484e87ee@cs.ucla.edu> <da92fb1c-9144-f2ee-b23c-1e98d88a63ef@redhat.com> <22a21507-adc7-3116-813e-0ee873e2a8f9@cs.ucla.edu> <dee2e7a1-24a4-ea3c-9cf5-d64fe30dcf22@redhat.com> <f0040461-1c1f-56c3-9b77-8593e33b34e3@cs.ucla.edu> <30c017fe-fafe-d787-7d76-865223cef599@linaro.org> <mvmfubi6clf.fsf@suse.de> From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Message-ID: <0e2a0dfe-5a44-f365-9544-ee0fc2b0df63@linaro.org> Date: Tue, 19 Sep 2017 14:34:59 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <mvmfubi6clf.fsf@suse.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Adhemerval Zanella
Sept. 19, 2017, 5:34 p.m. UTC
On 19/09/2017 10:29, Andreas Schwab wrote: > On Sep 19 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h >> index bb04662..0f49dd9 100644 >> --- a/include/scratch_buffer.h >> +++ b/include/scratch_buffer.h >> @@ -66,7 +66,7 @@ >> struct scratch_buffer { >> void *data; /* Pointer to the beginning of the scratch area. */ >> size_t length; /* Allocated space at the data pointer, in bytes. */ >> - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; >> + union { max_align_t __a; char __c[1024]; } __space; > > If you make it an anonymous union you don't even need to change the rest > of the code. > > Andreas. > Something like below maybe? --- * include/scratch_buffer.h (scratch_buffer): Use an union instead of a max_align_t array for __space definition. ---
Comments
On 09/19/2017 07:34 PM, Adhemerval Zanella wrote: > - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; > + union { max_align_t __align; char __space[1024]; }; Does gnulib require compiler support for anonymous unions? (BTW, I view this kind of work largely as a distraction, which is why I'm against keeping things in sync with gnulib.) Thanks, Florian
On 09/19/2017 11:56 AM, Florian Weimer wrote: > Does gnulib require compiler support for anonymous unions? Unfortunately, no. Gnulib is intended to run on strict C99 and other platforms that do not support anonymous unions. There should be no trouble sticking with ordinary unions here, as all this stuff is private to scratch_buffer and the code is simple either way. > > (BTW, I view this kind of work largely as a distraction, which is why > I'm against keeping things in sync with gnulib.) Many of the recently-installed fixes came from Gnulib, and I hope that more such fixes will be on the way, so there is a practical advantage to staying in sync when that is reasonably easy, as it is here.
On 19/09/2017 16:16, Paul Eggert wrote: > On 09/19/2017 11:56 AM, Florian Weimer wrote: >> Does gnulib require compiler support for anonymous unions? > > Unfortunately, no. Gnulib is intended to run on strict C99 and other platforms that do not support anonymous unions. There should be no trouble sticking with ordinary unions here, as all this stuff is private to scratch_buffer and the code is simple either way. > >> >> (BTW, I view this kind of work largely as a distraction, which is why I'm against keeping things in sync with gnulib.) > > Many of the recently-installed fixes came from Gnulib, and I hope that more such fixes will be on the way, so there is a practical advantage to staying in sync when that is reasonably easy, as it is here. > Although I do see value in keep in sync with gnulib, with the cost of live with gnulib code constraints; the changes you mentioned came mostly because you pushed them without much discussion on libc-alpha and I decided to sync the changes back. Also, this very change we are discussing in another one you take from glibc and adjusted to gnulib without actually discuss it back on libc-alpha. I would expect, to make it easier for both glibc and gnulib, that once a patch is submitted on a maillist we finish consensus and pushed on the referred project and *after* we sync with either glibc/gnulib. It is hard and waster a lot of time trying to sync with gnulib once one push patches without much discussion on libc-alpha. However I would to iterate that I do appreciate the work you did on glob patches reviews, I would just like to ask if we can get a little more consensus on glibc part before actually commit the patches.
Adhemerval Zanella wrote: > the changes you mentioned came mostly because you pushed > them without much discussion From glibc's point of view I suggest thinking of Gnulib as being the "farm team". Gnulib is largely maintained by people who are drawn to it partly because it is simpler and doesn't have all the bureaucratic hassles that glibc does, so that one can fix bugs and add features more quickly and easily than one can in glibc. There are advantages to both projects' development styles of course. > I would expect, to make it easier for both glibc and gnulib, that once a patch > is submitted on a maillist we finish consensus and pushed on the referred > project and *after* we sync with either glibc/gnulib. I doubt whether we Gnulib folks would want to have a policy of waiting for the usual days (weeks?) for a glibc review before installing a glob fix. Often we have a software release to put out, and Gnulib is supposed to help, not to slow us down. With that in mind, perhaps it's better to continue keeping the collaboration close but not lock-step, so that the goal is for glob.c etc. to be identical in both projects, but we can tolerate short periods when this is not true. That being said, there's no rush in installing into Gnulib the current set of patches that you're proposing, and it's fine to wait for you to revise it along the lines suggested before installing it into both Gnulib and glibc.
On 19/09/2017 22:20, Paul Eggert wrote: > Adhemerval Zanella wrote: >> the changes you mentioned came mostly because you pushed >> them without much discussion > > From glibc's point of view I suggest thinking of Gnulib as being the "farm team". Gnulib is largely maintained by people who are drawn to it partly because it is simpler and doesn't have all the bureaucratic hassles that glibc does, so that one can fix bugs and add features more quickly and easily than one can in glibc. > > There are advantages to both projects' development styles of course. > >> I would expect, to make it easier for both glibc and gnulib, that once a patch is submitted on a maillist we finish consensus and pushed on the referred >> project and *after* we sync with either glibc/gnulib. > > I doubt whether we Gnulib folks would want to have a policy of waiting for the usual days (weeks?) for a glibc review before installing a glob fix. Often we have a software release to put out, and Gnulib is supposed to help, not to slow us down. With that in mind, perhaps it's better to continue keeping the collaboration close but not lock-step, so that the goal is for glob.c etc. to be identical in both projects, but we can tolerate short periods when this is not true. > > That being said, there's no rush in installing into Gnulib the current set of patches that you're proposing, and it's fine to wait for you to revise it along the lines suggested before installing it into both Gnulib and glibc. What I am proposing based on recent glob fixes is to just align a bit more with glibc and avoid push one side changes for the shared code. For instance, on this very change instead of push a slight modified version, it would be more productive if you just send a message stating your intention is to push this modified version and give us some days to see if anyone have anything to say. If thread get stalled, I think it safe to proceed on gnulib side and let us deal with any glibc requirement once we plan to sync it back. Now back to the patch topic, I think the first union version [1] should the most suitable one and it is what I am intended to push. [1] https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html
diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h index bb04662..cc394a7 100644 --- a/include/scratch_buffer.h +++ b/include/scratch_buffer.h @@ -66,7 +66,7 @@ struct scratch_buffer { void *data; /* Pointer to the beginning of the scratch area. */ size_t length; /* Allocated space at the data pointer, in bytes. */ - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; + union { max_align_t __align; char __space[1024]; }; }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space