[2/2] Use C11 _Alignas on scratch_buffer internal buffer

Message ID 0e2a0dfe-5a44-f365-9544-ee0fc2b0df63@linaro.org
State Dropped
Headers

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

Florian Weimer Sept. 19, 2017, 6:56 p.m. UTC | #1
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
  
Paul Eggert Sept. 19, 2017, 7:16 p.m. UTC | #2
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.
  
Adhemerval Zanella Sept. 19, 2017, 7:57 p.m. UTC | #3
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.
  
Paul Eggert Sept. 20, 2017, 1:20 a.m. UTC | #4
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.
  
Adhemerval Zanella Sept. 21, 2017, 9:33 p.m. UTC | #5
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
  

Patch

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