Skip to content

Move SWIFT_RETURNS_NONNULL and SWIFT_NODISCARD attributes after the function name for exported functions. #61473

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Move SWIFT_RETURNS_NONNULL and SWIFT_NODISCARD attributes after the function name for exported functions. #61473

wants to merge 1 commit into from

Conversation

allevato
Copy link
Member

@allevato allevato commented Oct 6, 2022

More recent versions of Clang (~July 2022) have stricter enforcement of the positioning of attributes; for example, the nonnull and nodiscard attributes can no longer precede the extern "C" specifier and must follow the name of the function (and not the signature of the function) to which they apply.

This appears to just affect functions exported with extern "C"; other functions using these attributes can still have these attributes first without issue. 🤷🏻‍♂️

I have mixed feelings about the formatting here, but it's what clang-format did.

@allevato
Move SWIFT_RETURNS_NONNULL and SWIFT_NODISCARD attributes after t…
2963c38 
…he function name for exported functions.

More recent versions of Clang (~July 2022) have stricter enforcement of
the positioning of attributes; for example, the `nonnull` and `nodiscard`
attributes can no longer precede the `extern "C"` specifier and must
follow the name of the function (and not the signature of the function)
to which they apply.
@allevato allevato requested a review from compnerd October 6, 2022 18:11
@allevato
Copy link
Member Author

allevato commented Oct 6, 2022

@swift-ci please test

compnerd
compnerd reviewed Oct 6, 2022
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I really am tempted to say that we should ignore clang-format here. The interleaving already makes the functions more difficult to locate, but the formatting with ^SWIFT_NODISCARD( just is a whole different level of terrible.

I am also wondering if it makes sense to have a SWIFT_NONNULL_NODISCARD to help the flow of the text.

include/swift/Runtime/HeapObject.h
size_t requiredAlignmentMask);
SWIFT_RUNTIME_EXPORT
HeapObject *swift_allocObject SWIFT_RETURNS_NONNULL
SWIFT_NODISCARD(HeapMetadata const *metadata, size_t requiredSize,
Copy link
Member

Choose a reason for hiding this comment

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

Are you okay clang-format?

@compnerd
Copy link
Member

compnerd commented Oct 6, 2022

the nonnull and nodiscard attributes can no longer precede the extern "C" specifier and must follow the name of the function (and not the signature of the function) to which they apply.

This implies that this is occurring only in the C++ code path. Perhaps another, more invasive option, is to introduce an extern "C" scope for the functions to avoid interleaving the attributes.

CC: @etcwilde @bnbarham - they may have valuable insight/opinions given the previous experience with rebranching

@allevato
Copy link
Member Author

allevato commented Oct 6, 2022

I really am tempted to say that we should ignore clang-format here. The interleaving already makes the functions more difficult to locate, but the formatting with ^SWIFT_NODISCARD( just is a whole different level of terrible.

I am also wondering if it makes sense to have a SWIFT_NONNULL_NODISCARD to help the flow of the text.

It probably doesn't help that they're wrapped in macros so clang-format doesn't see them as attributes but as... something else.

I don't know how I feel about adding Yet Another Macro™ to this just to make the formatting slightly improved, since if someone needs to add more attributes to a particular function, we're just back to the same situation again.

This implies that this is occurring only in the C++ code path. Perhaps another, more invasive option, is to introduce an extern "C" scope for the functions to avoid interleaving the attributes.

CC: @etcwilde @bnbarham - they may have valuable insight/opinions given the previous experience with rebranching

I'd be quite happy to do something like that if it ends up cleaner!

@compnerd
Copy link
Member

compnerd commented Oct 6, 2022

I find it a bit more palatable, but that is just my opinion. #61476

Let us see what the others think.

@bnbarham
Copy link
Contributor

bnbarham commented Oct 6, 2022

I find it a bit more palatable, but that is just my opinion. #61476

Let us see what the others think.

That's a +1 from me. It's ... unfortunate. But the interleaving is quite awful 😅. My main question here is whether or not it's intentional that there's a behavior difference between extern "C" { ... } and extern "C" on the decl. Ie. it's odd that

must follow the name of the function (and not the signature of the function) to which they apply
changes in these cases.

If it is intentional then I would prefer @compnerd's PR. Otherwise this seems like the only option, though it would be nice to ignore clang-format here (or have clang-format treat those macros as attributes somehow).

@allevato
Copy link
Member Author

allevato commented Oct 6, 2022

If there aren't any other odd effects from @compnerd's PR, I definitely agree it's the better/cleaner option here.

@compnerd
Copy link
Member

compnerd commented Oct 6, 2022 •
edited
Loading

I'll try to find out about the behavioural differences and if this is truly intentional. Once we have that we can decide how to proceed.

compnerd
compnerd requested changes Oct 7, 2022
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I think that this change is not the right approach. The problem stems from the fact that the linkage is being intertwined with the attributes due to the macros being mis-ordered. Please see #61489

@allevato allevato mentioned this pull request Oct 7, 2022
Closed
TiaMonique
TiaMonique approved these changes Oct 12, 2022
View reviewed changes
include/swift/Runtime/Heap.h
@@ -29,8 +29,9 @@ namespace swift {
// Never returns nil. The returned memory is uninitialized.
//
// An "alignment mask" is just the alignment (a power of 2) minus 1.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
Copy link

Choose a reason for hiding this comment

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

include/swift/Runtime/Heap.h

TiaMonique
TiaMonique reviewed Oct 12, 2022
View reviewed changes
Copy link

@TiaMonique TiaMonique left a comment

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

include/swift/Runtime/Heap.h
@@ -29,8 +29,9 @@ namespace swift {
// Never returns nil. The returned memory is uninitialized.
//
// An "alignment mask" is just the alignment (a power of 2) minus 1.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
Copy link

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

include/swift/Runtime/HeapObject.h
@@ -61,10 +61,10 @@ struct OpaqueValue;
///
/// POSSIBILITIES: The argument order is fair game. It may be useful
/// to have a variant which guarantees zero-initialized memory.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
HeapObject *swift_allocObject(HeapMetadata const *metadata,
size_t requiredSize,
Copy link

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

include/swift/Runtime/HeapObject.h
@@ -117,8 +117,8 @@ BoxPair swift_makeBoxUnique(OpaqueValue *buffer, Metadata const *type,
size_t alignMask);

/// Returns the address of a heap object representing all empty box types.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
Copy link

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

include/swift/Runtime/HeapObject.h
@@ -117,8 +117,8 @@ BoxPair swift_makeBoxUnique(OpaqueValue *buffer, Metadata const *type,
size_t alignMask);

/// Returns the address of a heap object representing all empty box types.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
HeapObject* swift_allocEmptyBox();
Copy link

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

include/swift/Runtime/HeapObject.h
@@ -117,8 +117,8 @@ BoxPair swift_makeBoxUnique(OpaqueValue *buffer, Metadata const *type,
size_t alignMask);

/// Returns the address of a heap object representing all empty box types.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
HeapObject* swift_allocEmptyBox();
Copy link

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

include/swift/Runtime/HeapObject.h
@@ -117,8 +117,8 @@ BoxPair swift_makeBoxUnique(OpaqueValue *buffer, Metadata const *type,
size_t alignMask);

/// Returns the address of a heap object representing all empty box types.
SWIFT_RETURNS_NONNULL SWIFT_NODISCARD SWIFT_RUNTIME_EXPORT
HeapObject* swift_allocEmptyBox();
Copy link

Choose a reason for hiding this comment

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

allevato:shuffle-clang-attributes

@TiaMonique
Copy link

allevato:shuffle-clang-attributes

@edymtt edymtt mentioned this pull request Oct 25, 2022
Closed
@compnerd
Copy link
Member

There is a fix up for llvm/llvm-project#58229 (https://reviews.llvm.org/D137979)

@allevato
Copy link
Member Author

Closing this in favor of the Clang fix.

@allevato allevato closed this Nov 16, 2022
@allevato allevato deleted the shuffle-clang-attributes branch November 16, 2022 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TiaMonique TiaMonique TiaMonique approved these changes

@compnerd compnerd compnerd requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@allevato @compnerd @bnbarham @TiaMonique