Replaced aligned/algin compiler attributes with C++ 11 alignas

[kohei306]

No description provided.

[derekmauro]

Thank you for the pull request. It would be nice if we could accept this, but the problem we have is that this file is included in some Google-internal files that are compiled in C, not C++. Maybe one day this can be cleaned-up.

[kohei306]

Thank you for the pull request. It would be nice if we could accept this, but the problem we have is that this file is included in some Google-internal files that are compiled in C, not C++. Maybe one day this can be cleaned-up.

I see. Thanks for reviewing anyway.
Then does it make sense to change the comment to reflect the situation, maybe adding what you wrote "this file is included in some Google-internal files that are compiled in C, not C++. Maybe one day this can be cleaned-up."

[mbxx]

Yes, at a minimum this needs to be documented. I don't have a sense of how big the cleanup would be, though, so it might be possible to just do it. I'll check out what needs to be done and get back to you next week.

[mbxx]

My apologies for the slow response, this looks like a bigger cleanup than I'd suspected.

The files that are compiled as both C and C++ turn out not to be a problem, or at least not something I see as a problem through the other problems I've run into.

The first is the attribute is accepted in more positions than the alignas specifier. That means there's a lot of mechanical cleanup needed pushing ABSL_CACHELINE_ALIGNEDs around to avoid widespread build breakages.

The second is alignas just can't be applied to everything that ABSL_CACHELINE_ALIGNED is currently applied to. There's an example in TCMalloc: https://github.com/google/tcmalloc/blob/c06632feb8dedbd7cf41666b50c8a98f92837904/tcmalloc/tcmalloc.cc#L1914 Here it's being applied to a function's return value. It's not obvious what the best way to fix that would be. Offhand, I suspect the attribute will still be needed.

I'll start a conversation in the team to see what we'd like to do.

[kohei306]

Hi @mbxx, Thanks for taking a deep look into it. I was not aware of the difference in the accepted position between them.
It sounds like the clean up takes some efforts as you pointed out and I cannot tell if it is worthwhile the effort. Maybe if the usages of ABSL_CACHELINE_ALIGNED for things that cannot be replaced with alignas are minor cases and they can be swapped for the majority of the cases, it is a shame that the clean up is blocked due to the minor cases.
Of course even then, the clean up steps should be done in a way no to break the minor cases. Anyway, thanks for taking it to team discussion.

[mbxx]

I'm not actually scared of the internal cleanup: it's a few hundred files to fix, which won't actually take me that long, and making Abseil more portable is a good thing. The documentation in the header file also explicitly warns about the placement of the attribute explicitly, so I believe this change is okay for open source users. I think this PR is the right direction to go in.

I seem to have been wrong about what the TCMalloc usage I mentioned before (https://github.com/google/tcmalloc/blob/c06632feb8dedbd7cf41666b50c8a98f92837904/tcmalloc/tcmalloc.cc#L1914) was really doing: it seems to be an attribute on the function (https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes). We still need to figure out what to do in cases like that, and I'm afraid folks have started to take time off for the holidays. I'll keep pushing on this and post an update in January. Thanks for your patience :)

[kohei306]

Hi @mbxx,
Yes, I agree that it will make Abseil more portable by adhering to standard. Thanks for sharing the link. It looks very informative.
Sure, I can totally understand that it is better to wait till people come back from holidays :-) Thanks again for looking into that!

[mbxx]

My apologies for letting this go so long. We chatted about this and came to the conclusion that the implementation is going to be difficult to change and a better route for callers to migrate from ABSL_CACHELINE_ALIGNED to the equivalent C++17 idioms when they're able to do so. We haven't quite decided what to do about ABSL_CACHELINE_ALIGNED itself in the long term (maybe it continues to exist in its current form or maybe it becomes deprecated), but there's no pressing need to come to a decision (Abseil supports C++11 so we'll need ABSL_CACHELINE_ALIGNED for the foreseeable future).

Sound reasonable to you?

If so, the next step is for me to update the comment to reflect this plan.

[kohei306]

Hi @mbxx ,
(sorry, I closed the PR by mistake.) No problem. Yes, I agree that we reflect the outcome of our discussion/your investigation to the comment so that it does not go wasted and then keep the existing code as it is. Thanks for your investigation and explanation, it definitely helped to understand the context for external contributors :-)

[mbxx]

Cool, I've update the comment here:

abseil-cpp/absl/base/optimization.h

Lines 109 to 112 in 1fdbe1e

	// NOTE: callers should replace uses of this macro with `alignas()` using
	// `std::hardware_constructive_interference_size` and/or
	// `std::hardware_destructive_interference_size` when C++17 becomes available to
	// them.

I hope this helps. If not, please let me know how I could tweak it to make it a little more clear.