Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

place alignas() before lifetime specifiers #11248

Closed
wants to merge 3 commits into from

Conversation

mumbleskates
Copy link
Contributor

This patch fixes a compilation bug introduced in 821b073.

When the constinit keyword is available (such as in C++20), putting the alignas attribute after ABSL_CONST_INIT is an error:

ERROR: /home/widders/.cache/bazel/_bazel_widders/25f0d335ca0e01a2fd74c60e90175e8f/external/com_google_protobuf/src/google/protobuf/compiler/java/BUILD.bazel:44:11: Compiling src/google/protobuf/compiler/java/generator_factory.cc failed: (Exit 1): clang failed: error executing command /usr/lib/llvm-16/bin/clang -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer -g0 ... (remaining 64 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
In file included from external/com_google_protobuf/src/google/protobuf/compiler/java/generator_factory.cc:35:
In file included from bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/src/google/protobuf/compiler/java/_virtual_includes/java/google/protobuf/compiler/java/context.h:38:
In file included from bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/src/google/protobuf/compiler/java/_virtual_includes/names_internal/google/protobuf/compiler/java/helpers.h:45:
In file included from bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/protobuf_nowkt/google/protobuf/descriptor.pb.h:25:
In file included from bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena.h:53:
bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena_impl.h:584:19: error: an attribute list cannot appear here
  ABSL_CONST_INIT alignas(
                  ^~~~~~~~
1 error generated.

Therefore, it should be ordered as

  1. always attribute-like (alignas)
  2. sometimes attributes (ABSL_CONST_INIT, which may be a lifetime specifier or an attribute)
  3. lifetime specifiers (static)
  4. type etc.

Where it currently is it may be in the midst of lifetime specifiers, and if it is moved to the right it syntactically applies to the type rather than the whole declaration which is also invalid.

If this ordering could not be resolved for all cases it could also be moved to the end, after the name being declared, but we don't need to do that here.

Copy link
Contributor

@jorgbrown jorgbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! Just one nit about spacing.

src/google/protobuf/arena_impl.h Outdated Show resolved Hide resolved
Copy link
Contributor

@jorgbrown jorgbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants