[RFC] Allow packed types to transitively contain aligned types #3718
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| - Feature Name: `layout_packed_aligned` | ||
| - Start Date: 2024-10-24 | ||
| - RFC PR: [rust-lang/rfcs#3718](https://github.com/rust-lang/rfcs/pull/3718) | ||
| - Rust Issue: [rust-lang/rust#100743](https://github.com/rust-lang/rust/issues/100743) | ||
|
|
||
| # Summary | ||
| [summary]: #summary | ||
|
|
||
| This RFC makes it legal to have `#[repr(C)]` structs that are: | ||
| - Both packed and aligned. | ||
| - Packed, and transitively contains`#[repr(align)]` types. | ||
|
|
||
| It also introduces `#[repr(system)]` which is designed for interoperability with operating system APIs. | ||
| It has the same behavior as `#[repr(C)]` except on `*-pc-windows-gnu` targets where it uses the msvc layout | ||
| rules instead. | ||
|
|
||
| # Motivation | ||
| [motivation]: #motivation | ||
|
|
||
| This RFC enables the following struct definitions: | ||
|
|
||
| ```rs | ||
| #[repr(C, packed(2), align(4))] | ||
| struct Foo { // Alignment = 4, Size = 8 | ||
| a: u8, // Offset = 0 | ||
| b: u32, // Offset = 2 | ||
| } | ||
| ``` | ||
|
|
||
| This is commonly needed when Rust is being used to interop with existing C and C++ code bases, which may contain | ||
| unaligned types. For example in `clang` it is possible to create the following type definition, and there is | ||
| currently no easy way to create a matching Rust type: | ||
|
|
||
| ```cpp | ||
| struct __attribute__((packed, aligned(4))) MyStruct { | ||
|
There was a problem hiding this comment. It is somewhat confusing that the Rust example uses |
||
| uint8_t a; | ||
| uint32_t b; | ||
| }; | ||
Neo-Zhixing marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ``` | ||
|
|
||
| Currently, `#[repr(packed(_))]` structs cannot be `#[repr(align(_))]` or transitively contain `#[repr(align(_))]` types. Attempting to do so results in a [hard error](https://doc.rust-lang.org/nightly/error_codes/E0588.html). | ||
|
|
||
| This behavior was added in the [original implementation](https://github.com/rust-lang/rust/issues/33158) of `#[repr(packed)]` due to concerns over differing behavior between msvc and gcc/clang. This makes it cumbersome or even impossible to produce C-compatible struct layouts in Rust when the corresponding C types were annotated with both `packed` and `aligned`. | ||
|
|
||
| # Guide-level explanation | ||
| [guide-level-explanation]: #guide-level-explanation | ||
|
|
||
| ## `#[repr(C)]` | ||
| When `align` and `packed` attributes exist on the same type, or when `packed` structs transitively contains `align` types, | ||
CAD97 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Is there any difference in behavior between the c, system, and rust for types with both packed and align? My expectation would be that regardless of whether it is c, system, or rust, if I have And FWIW, it would be more intuitive to me if packed ignored the alignment of field types, but had a way to specify higher alignment for individual fields. There was a problem hiding this comment. As currently written, In MSVC C, such would theoretically be a compilation error ( This is consistent with the behavior of |
||
| the resulting layout matches the target toolchain ABI. | ||
|
There was a problem hiding this comment. To verify, Clang/LLVM does replicate the MSVC behavior correctly for its MSVC target, correct? It's a harder sell to change There was a problem hiding this comment. I do believe so. https://clang.llvm.org/docs/MSVCCompatibility.html
|
||
|
|
||
| For example, given: | ||
| ```c | ||
| #[repr(C, align(4))] | ||
| struct Foo(u8); | ||
| #[repr(C, packed(1))] | ||
| struct Bar(Foo); | ||
| ``` | ||
| `align_of::<Bar>()` would be 4 for `*-pc-windows-msvc` and 1 for everything else. | ||
|
There was a problem hiding this comment. IMO this is extremely unintuitive behavior, especially for those of us writing custom derives (like zerocopy). We can't just rely on querying Also, while this is technically not a breaking change, I wouldn't be surprised if some custom derive code implicitly assumes that this behavior isn't possible, and would become unsound in the face of this change. For example, as it stands today, it is valid to assume that a I'd propose that if system-specific behavior like this is required, it'd be better to do it behind a new repr so that the behavior of There was a problem hiding this comment.
Did you say this wrong somehow? By this RFC, What changes is that
Such custom derive code is already likely quite iffy, since There was a problem hiding this comment.
You're right, I misread the text. Still, there's a concern: Our derives currently take
IIUC, to make There's another concern as well: This makes "whether or not the type has an alignment repr ( #[repr(C, align(4))]
struct Foo(u8);
#[repr(C, packed(1))]
struct Bar(Foo);
#[repr(C)]
struct Foo(u8, [u32; 0]);
#[repr(C, packed(1))]
struct Bar(Foo);There was a problem hiding this comment.
This does not compile today (reference said "a There was a problem hiding this comment. My point isn't that this is a breaking change, but rather than it breaks the current mental model, which is that a type's layout is determined solely by the size and alignment of its fields. This RFC changes that to say that a type's layout is determined by the size, alignment, and representation of its fields. Since representation is a fairly complicated concept (it can include repr(C), repr(Int), repr(packed), repr(align), repr(transparent), and some-but-not-all combinations of these), IMO it significantly complicates the mental model of type layout to say that a type's layout also depends upon its fields' representations. There was a problem hiding this comment. Unfortunately, the existing. mental model turned out to be in contradiction to reality, at least on MSVC targets. When you build a model and then reality demonstrates the incorrectness of the model, sometimes the best option is to fix your model. Yes we documented this model in a bunch of places, but... what else could we do? Stick our head into the sand and continue pretending that the simpler model is "good enough"? So now the layout of a type is determined by the size, alignment, and explicitly requested alignment of its fields. There was a problem hiding this comment.
There is another alternative: argue that because Counter to my own point, though, is that But if there exists a type on MSVC with basic alignment higher than the default member packing level without also having an explicitly requested alignment (thus not being sufficiently aligned by default), then I would swing towards writing of MSVC layout edge cases as broken.
Unfortunately, this error is straightforward to sidestep with generics. #[repr(align(4))]
struct Aligned;
#[repr(packed(1))]
struct Packed<T = Aligned>(T);
fn main() {
dbg!(align_of::<Packed>());
// [src/main.rs:8:5] align_of::<Packed>() = 1
}There was a problem hiding this comment. Isn't it the case that all of these issues could be avoided by introducing a new repr rather than changing the meaning of There was a problem hiding this comment. The counter argument is that a similarly huge amount of This RFC's position is that My personal position is that |
||
|
|
||
|
|
||
| ## `#[repr(system)]` | ||
| When `align` and `packed` attributes exist on the same type, or when `packed` structs transitively contains `align` types, | ||
| the resulting layout matches the target OS ABI. | ||
|
|
||
| For example, given: | ||
| ```c | ||
| #[repr(system, align(4))] | ||
| struct Foo(u8); | ||
| #[repr(system, packed(1))] | ||
| struct Bar(Foo); | ||
| ``` | ||
| `align_of::<Bar>()` would be 4 for `*-pc-windows-msvc` and `*-pc-windows-gnu`. It would be 1 for everything else. | ||
|
|
||
| ## `#[repr(Rust)]` | ||
|
There was a problem hiding this comment. When would someone want this on ( There was a problem hiding this comment. We do allow standalone |
||
| When `align(N)` and `packed(M)` attributes exist on the same type, or when `packed` structs contain `aligned` fields, | ||
| the type will have their base alignment increased to `N`, while the struct fields will be laid out as if their | ||
| alignments were decreased to `M`. However, in general Rust is free to reorder | ||
| these fields for optimization purposes, and the only guarantee is that the fields will maintain a minimum alignment of `M`. | ||
|
|
||
| # Reference-level explanation | ||
| [reference-level-explanation]: #reference-level-explanation | ||
|
|
||
| In the following paragraphs, "Decreasing M to N" means: | ||
| ``` | ||
| if M > N { | ||
| M = n | ||
| } | ||
| ``` | ||
|
|
||
| "Increasing M to N" means: | ||
| ``` | ||
| if M < N { | ||
| M = N | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| `#[repr(align(N))]` increases the base alignment of a type to be N. | ||
|
|
||
| `#[repr(packed(M))]` decreases the alignment of the struct fields to be M. Because the base alignment of the type | ||
| is defined as the maximum of the alignment for any fields, this also has the indirect result of decreasing the base | ||
| alignment of the type to be M. | ||
|
|
||
| When the align and packed modifiers are applied on the same type as `#[repr(align(N), packed(M))]`, | ||
| the alignment of the struct fields are decreased to be M. Then, the base alignment of the type is | ||
| increased to be N. | ||
|
|
||
| When a `#[repr(packed(M))]` struct transitively contains a field with `#[repr(align(N))]` type, depending on the | ||
| target triplet, either: | ||
|
There was a problem hiding this comment. This should IMO be rephrased as an algorithm that works one "layer" at a time. Computing the layout of a type T should only consider the fields of T and their properties. It should never recurse into the fields of T. I don't currently actually understand the proposed spec here, and this should make it a lot easier to understand. I suspect what will happen is that as part of this, we will have to introduce a new property of T that is "bubbled up" in the recursion -- a new degree of freedom that was not required so far. (@CAD97 has already alluded to this elsewhere.) Identifying and clearly describing this property will make it a lot easier to understand the resulting layout algorithm. There was a problem hiding this comment. My current understanding of the algorithm is as follows: We equip types with a new property, the "explicitly requested alignment". For all base types, this alignment is 1. For structs, it is by default the maximum of the explicitly requested alignments of all fields. For a struct with the Note that the explicitly requested alignment of a type can never be bigger than the required alignment of the type. When computing the layout of a packed(P) struct, then currently we ensure each field is aligned to I am not fully confident that this is correct. Here's a corner case: #[repr(C, align(4))]
struct Align4(i32);
#[repr(C, align(2))]
struct Align2(Align4);
#[repr(C, packed)]
struct Packed(Align2);What is the resulting alignment of There was a problem hiding this comment. Another corner case: #[repr(C, align(4))]
struct Align4(i32);
struct Group(u8, Align4);
#[repr(C, packed)]
struct Packed(u16, Group);What does the resulting layout of
Is that correct? There was a problem hiding this comment. (The post this referred to has since been deleted.) Oh, so having an There was a problem hiding this comment. https://c.godbolt.org/z/es1cGPhz9 On MSVC 19.40 (VS 17.10) in C mode, #include <stdalign.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
__declspec(align(4))
struct Align4
{
int32_t _0;
};
__declspec(align(2))
struct Align2
{
struct Align4 _0;
};
#pragma pack(push, 1)
struct Packed
{
struct Align2 _0;
};
#pragma pack(pop)
struct Group
{
uint8_t _0;
struct Align4 _1;
};
#pragma pack(push, 1)
struct P
{
uint16_t _0;
struct Group _1;
};
#pragma pack(pop)
int main()
{
printf("alignof(Packed) = %zu\n", alignof(struct Packed));
printf("\n");
printf("offsetof(P, _0) = %zu\n", offsetof(struct P, _0));
printf("offsetof(P, _1) = %zu\n", offsetof(struct P, _1));
printf("offsetof(P, _1._0) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _0));
printf("offsetof(P, _1._1) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _1));
}gives Using
and results of 1 / 0, 2, 2, 6; fully just ignoring the along with a warning:
EDIT TO ADD: interesting: in C mode, There was a problem hiding this comment. For completeness, clang does not seem to implement this fully, unless I made a mistake: [godbolt] #include <stdalign.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
struct [[gnu::ms_struct]] Align4
{
alignas(4)
int32_t _0;
};
struct [[gnu::ms_struct]] Align2
{
// alignas(2) // error: requested alignment is less than minimum alignment of 4 for type 'struct Align4'
struct Align4 _0;
};
struct [[gnu::packed]] Packed
{
struct Align2 _0;
};
struct Group
{
uint8_t _0;
struct Align4 _1;
};
struct [[gnu::packed]] P
{
uint16_t _0;
struct Group _1;
};
int main()
{
printf("alignof(Packed) = %zu\n", alignof(struct Packed));
printf("\n");
printf("offsetof(P, _0) = %zu\n", offsetof(struct P, _0));
printf("offsetof(P, _1) = %zu\n", offsetof(struct P, _1));
printf("offsetof(P, _1._0) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _0));
printf("offsetof(P, _1._1) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _1));
} |
||
| - The field is first `pad_to_align`. Then, the field is added to the struct with alignment decreased to M. The packing requirement overrides the alignment requirement. (GCC, `#[repr(Rust)]`, `#[repr(C)]` on gnu targets, `#[repr(system)]` on non-windows targets), or | ||
|
There was a problem hiding this comment. It seems like the parenthetical here is meant to say when which case applies? That's quite confusing, please state the condition before the consequence -- just like in code, |
||
| - The field is added to the struct with alignment increased to N. The alignment requirement overrides the packing requirement. (MSVC, `#[repr(C)]` on msvc targets, `#[repr(system)]` on windows targets) | ||
|
There was a problem hiding this comment. The RFC should be explicit about the fact that this introduces a new kind of alignment to Rust type layout. There's the existing Is this new alignment flavor exposed to code in any way? If so, is it included in There was a problem hiding this comment. We're not introducing a new type of alignment. Conceptually:
When both rules apply to the same type, (that's when a packed type transitively contains an aligned field), we have ambiguity, and we need some rules to determine who wins. So really we're just changing the value returned by There was a problem hiding this comment. But it is a different kind of alignment. There was a problem hiding this comment. To reiterate this point: you end up with "primitive alignment" which can be suppressed by Types are not constructed as a list of primitive fields flattened from the relevant types, even as much as the MSVC layout algorithm sometimes pretends that this is the case. Types are instead defined compositionally, and a field of a struct or enum type is no different from that of some primitive type. There was a problem hiding this comment. And here "user alignment" would be what is ultimately returned by This reminds me a lot of the discussions around There was a problem hiding this comment. I think any new kind of alignment should be exposed to user code specifically because user code may want to have its own layout algorithm (e.g. for JIT). Maybe add it as a new field to There was a problem hiding this comment. Adding fields to There was a problem hiding this comment.
pub struct OldLayout {
size: usize,
align: usize, // really a wrapper of a repr(usize) enum
}
pub struct NewLayout {
size: usize,
log2_align: u8,
#[cfg(windows)] // or any other platforms that have msvc's weirdness
log2_manual_align: u8,
}There was a problem hiding this comment.
ok, maybe make a There was a problem hiding this comment. actually, now that I'm thinking about it, you'd probably want
Neo-Zhixing marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
There was a problem hiding this comment. Due to this, this RFC will actually change the layout of some types that are currently accepted on stable, on MSVC targets. That should be discussed as a drawback. |
||
|
|
||
| Historically the meaning of `#[repr(C)]` has been somewhat ambiguous. When someone puts `#[repr(C)]` on their struct, their intention could be one of three things: | ||
|
There was a problem hiding this comment. https://doc.rust-lang.org/reference/type-layout.html#the-c-representation actually documents the meaning of That should at least be mentioned and discussed as a drawback. There was a problem hiding this comment. Isn't such a breaking change an explicit violation of Rust's stability policy? Though I can't actually find where it's documented ATM, my understanding is that breaking changes are only permitted in the following cases:
There was a problem hiding this comment. As described in #3718 (comment), it should be simple to avoid this problem by introducing a new repr (in addition to the proposed There was a problem hiding this comment. It's definitely in a gray area. One cold also argue that the existing There was a problem hiding this comment. iirc someone suggested deprecating |
||
| 1. Having a target-independent and stable representation of the data structure for storage or transmission. | ||
| 2. FFI with C and C++ libraries compiled for the same target. | ||
| 3. Interoperability with operating system APIs. | ||
|
|
||
| Today, `#[repr(C)]` is being used for all 3 scenarios because the user cannot create a `#[repr(C)]` struct with ambiguous layout between targets. However, this also means | ||
| that there exists some C layouts that cannot be specified using `#[repr(C)]`. | ||
|
|
||
| This RFC addresses use case 2 with `#[repr(C)]` and use case 3 with `#[repr(system)]`. For use case 1, people will have to seek alternative solutions such as `crABI` or | ||
| protobuf. However, it could be a footgun if people continue to use `#[repr(C)]` for use case 1. | ||
|
|
||
|
|
||
|
|
||
| # Rationale and alternatives | ||
| [rationale-and-alternatives]: #rationale-and-alternatives | ||
|
|
||
| This RFC clarifies that: | ||
| - `repr(C)` must interoperate with the C compiler for the target. | ||
| - `repr(system)` must interoperate with the operating system APIs for the target. | ||
| - Similiar to Clang, `repr(C)` does not guarantee consistent layout between targets. | ||
|
|
||
| Alternatively, we can also create syntax that allows the user to specify exactly which semantic to use when packed structs transitively contains aligned fields. | ||
| For example, a new attribute: #[repr(align_override_packed(N))] that can be used when the behavior of the child overriding the parent alignment is desired. | ||
|
|
||
| #[repr(align(N))] #[repr(packed)] can be used together to get the opposite behavior, parent/outer alignment wins. | ||
|
|
||
| Explicitly specifying the pack/align semantic has the drawback of complicating FFI. For example, you might need two different definition files depending on the target. | ||
|
|
||
| Therefore, a stable layout across compilation target should be relegated as future work. | ||
|
|
||
|
|
||
|
|
||
|
|
||
| # Prior art | ||
| [prior-art]: #prior-art | ||
|
|
||
| Clang matches the Windows ABI for `x86_64-pc-windows-msvc` and matches the GCC ABI for `x86_64-pc-windows-gnu`. | ||
|
|
||
| MinGW always uses the GCC ABI. | ||
|
There was a problem hiding this comment. Is there prior art for a compiler that can lay out types both using the Windows ABI and the GCC ABI for code within a single target? If yes, how are they distinguishing the two? If no, why does Rust need this ability? There was a problem hiding this comment. gcc apparently supports that by using the There was a problem hiding this comment. So that would correspond tom in Rust
There was a problem hiding this comment. For a more full parallel to There was a problem hiding this comment. |
||
|
|
||
| We already have both `C` and `system` [calling conventions](https://doc.rust-lang.org/beta/nomicon/ffi.html#foreign-calling-conventions) | ||
| to support differing behavior on `x86_windows` and `x86_64_windows`. | ||
|
|
||
|
|
||
| This issue was introduced in the [original implementation](https://github.com/rust-lang/rust/issues/33158) of `#[repr(packed(N))]` and have since underwent extensive community discussions: | ||
| - [#[repr(align(N))] fields not allowed in #[repr(packed(M>=N))] structs](https://github.com/rust-lang/rust/issues/100743) | ||
| - [repr(C) does not always match the current target's C toolchain (when that target is windows-msvc)](https://github.com/rust-lang/unsafe-code-guidelines/issues/521) | ||
| - [repr(C) is unsound on MSVC targets](https://github.com/rust-lang/rust/issues/81996) | ||
| - [E0587 error on packed and aligned structures from C](https://github.com/rust-lang/rust/issues/59154) | ||
| - [E0587 error on packed and aligned structures from C (bindgen)](https://github.com/rust-lang/rust-bindgen/issues/1538) | ||
| - [Support for both packed and aligned (in repr(C)](https://github.com/rust-lang/rust/issues/118018) | ||
| - [bindgen wanted features & bugfixes (Rust-for-Linux)](https://github.com/Rust-for-Linux/linux/issues/353) | ||
| - [packed type cannot transitively contain a #[repr(align)] type](https://github.com/rust-lang/rust-bindgen/issues/2179) | ||
| - [structure layout using __aligned__ attribute is incorrect](https://github.com/rust-lang/rust-bindgen/issues/867) | ||
|
|
||
|
|
||
| # Unresolved questions | ||
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| None for now. | ||
|
|
||
|
|
||
| # Future possibilities | ||
| [future-possibilities]: #future-possibilities | ||
|
|
||
| People intending for a stable struct layout consistent across targets would be directed to use `crABI`. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's be worth splitting this out.
I'm a big fan of splitting
repr(linear)vsrepr(C)(which I think this is spelling asrepr(C)vsrepr(system)) to have the distinction between "the layout you get with https://doc.rust-lang.org/beta/std/alloc/struct.Layout.html#method.extend" and "whatever weird layout your C compiler uses". That distinction would be really nice for making intent clearer, since today you get "you can't do that in C" warnings sometimes just because you usedrepr(C)to have a predictable layout for your Rust-only code.So I'd kinda like to consider that separately from any new packed-related stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the only difference between
#[repr(C)]and#[repr(system)]by this RFC is that on pc-windows-gnu#[repr(system)]is the MSVC layout while#[repr(C)]is the GCC layout. On all other targets#[repr(system)]and#[repr(C)]are identical (per this RFC).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
repr(linear)is independently useful and we should have all 3. that said, I am concerned that currentrepr(C)code often means "I want stable linear layout" rather than "I want whatever weirdness the C compiler decides to use", so I think deprecatingrepr(C)and replacing it withrepr(linear),repr(bikeshed_other_C)andrepr(system)is worth considering.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this RFC proposes a different distinction between
repr(C)andrepr(system)than what has been previously discussed in other threads.Since the distinction between the two layouts we have here is Windows-only, I wonder if it should be some Windows-only name, like
repr(msvc)or so? Is there a good reason to even make both of them available on all targets -- effectively exporting a Windows-only complication to other, saner platforms?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that this is also how the
extern "system"function ABI string works. Code usesextern "system"to link to libraries which use__stdcallon Windows platforms, and in the same way code would use#[repr(system)]for linking to a dylib which provides builds with only the MSVC toolchain for Windows targets.I don't necessarily endorse this option, but it is logically consistent with how Rust already uses
"system"as an ABI string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was previously forbidden but the check has some gaps, so this can impact existing code.
It also breaks existing code that assumes that all
repr(C)types are laid out according to the rules described here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand,
extern "system"is the same on MSVC and GNU Windows targets? So this is IMO a false analogy then, making it more confusing than if we instead use a name that more explicitly represents that Windows has two ABIs, which we support with two target triples, and you might want to write code that talks with the "other" ABI.Speaking of which, how would a program for the MSVC target lay out its type in the right way to call a GNU ABI library? That does not seem possible with this proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
extern "system"isextern "stdcall"on all Windows targets. Just like how this RFC makesrepr(system)on both windows-msvc and windows-gnu behave asrepr(MS). The difference isrepr(C), which switches between msvc/gnu respectively.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, I mixed up which is
Cand which issystemthen.The first repr in that sentence should be
system, notC, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes, that's correct.