On 02/27/17 16:31, Groke, Paul via Boost wrote:
While preparing a pull request for my Boost.Atomic extensions for the z/OS compiler, I noticed that I have a slight problem there. To get the code to work on z/OS, I changed the placement of the BOOST_ALIGNMENT macro in atomic/detail/storage_type.hpp from...
BOOST_ALIGNMENT(16) unsigned char data[Size];
to
unsigned char data[Size] BOOST_ALIGNMENT(16);
...because unfortunately the z compiler doesn't understand the __attribute__ ((__aligned__(x))) when it's at the beginning of a variable/data member definition. (For the GNU __attribute__ ((__aligned__(x))) the new location is also the only documented location, although GCC doesn't seem to mind.)
This makes the compiler incompatible with the current semantics of BOOST_ALIGNMENT. You probably need to revise the changes made in the recent Boost.Config PR[1] regarding BOOST_ALIGNMENT. Please, prepare an updating PR for Boost.Config.
While reviewing my changes I now realized that this won't do, because BOOST_ALIGNMENT can also be defined as __declspec(align(x)) or alignas(x), both of which are legal at the beginning but not at the end of a variable/data member definition. (I'm not 100% sure about valid positions of alignas (x) - the examples I can find use alignas (x) at the beginning, but the compilers we currently use - which include MSVC, GCC and Clang, also seem to accept it at the end. __declspec(align(x)) definitely is a problem though.)
Note also that BOOST_ALIGNMENT can (and is) also used as a type attribute. Does your compiler support that?
And now I'm wondering what to do about this.
One way to fix this would obviously be to introduce two additional macros, e.g. BOOST_ALIGNMENT_DECLSPEC and BOOST_ALIGNMENT_ATTRIBUTE. Only one of those would then expand to something non-empty, and both would have to be used in variable definitions. The BOOST_ALIGNMENT macro would remain as is for type declarations and for compatibility. The code in storage_type.hpp would then have to be re-written as
BOOST_ALIGNMENT_DECLSPEC(16) unsigned char data[Size] BOOST_ALIGNMENT_ATTRIBUTE(16);
Which of course is very very ugly.
Yeah, I don't really like the duplication.
Another option would be a BOOST_ALIGNED_VARIABLE(alignment, variable_definition) macro. This would be nice in definitions like
BOOST_ALIGNED_VARIABLE(16, unsigned char data[Size]);
but much less nice in definitions like
BOOST_ALIGNED_VARIABLE(16, some_template<foo BOOST_PP_COMMA bar BOOST_PP_COMMA baz>);
And since not all compilers support variadic macros, this is probably the best one could do.
I'm not sure BOOST_PP_COMMA would work. I'm not very good at preprocessor programming, but doesn't BOOST_PP_COMMA get expanded some time before BOOST_ALIGNED_VARIABLE? That would result in the latter being invoked with a variable number of arguments. A typedef could be used as a workaround, although it may be inconvenient in some cases.
The third option I can think of would be to leave things as they are, which of course would mean that I can't submit my Boost.Atomic implementation for z/OS... hm.
I realize that making things uglier for a single compiler that almost nobody uses isn't something many people will cheer for. But then again I'd really like to have my Boost.Atomic implementation for z/OS in the official Boost releases :)
So I'm calling out to you for suggestions/opinions/comments. Is there another way to combine the above mentioned square peg + round hole? And which option would you deem favorable/acceptable?
Frankly, I'm not quite happy with either option, although BOOST_ALIGNED_VARIABLE seems the lesser evil. One other idea that I think is worth trying is to see if your compiler can cope with alignment attribute in a type definition, in a usual placement. struct BOOST_ALIGNMENT(16) aligned { unsigned char data[Size]; }; If it does, the code can be changed to avoid applying BOOST_ALIGNMENT to variables and use it to align types instead. Although I used BOOST_ALIGNMENT above for shortness, I shouldn't be saying BOOST_ALIGNMENT because, as currently documented[2], BOOST_ALIGNMENT is not supported by your compiler and thus should expand to nothing (with BOOST_NO_ALIGNMENT defined). If your compiler supports type alignment in the above syntax, we could introduce a new macro, e.g. BOOST_TYPE_ALIGNMENT(n), that would be usable in that context, but not in the context of a variable declaration. It can be accompanied with BOOST_NO_TYPE_ALIGNMENT, similar to BOOST_NO_ALIGNMENT. If it doesn't work with types, then you could just sacrifice 128-bit atomic ops in Boost.Atomic. No changes to Boost.Atomic should be needed for that because the lesser types should already get the necessary alignment naturally, and 128-bit storage will be disabled by the current preprocessor checks. I think, that would be a good solution with minimal impact on the existing code unless, of course, you require 128-bit atomics. [1]: https://github.com/boostorg/config/pull/122 [2]: http://www.boost.org/doc/libs/1_63_0/libs/config/doc/html/boost_config/boost...