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

Unify group code snippet with other non-user-constructible classes #627

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlexeySachkov
Copy link
Contributor

group is not user-constructible, but explicit information about its constructors was missing from the code snippet.

`group` is not user-constructible, but explicit information about its
constructors was missing from the code snippet.
@gmlueck gmlueck added the editorial Some purely editorial problem label Sep 20, 2024
@gmlueck
Copy link
Contributor

gmlueck commented Sep 20, 2024

Adding the editorial label. The spec was already clear that these types are not user constructible, so this PR just clarifies that in the synopsis. I'll wait a few days before merging in case others have comments.

@keryell
Copy link
Member

keryell commented Sep 25, 2024

Just curious about this implementation detail leakage: I have the feeling that "not user-constructible" does not mean it has no default constructor, just it cannot be constructed by the user. For example the constructor could be private.

@gmlueck
Copy link
Contributor

gmlueck commented Sep 26, 2024

Just curious about this implementation detail leakage: I have the feeling that "not user-constructible" does not mean it has no default constructor, just it cannot be constructed by the user. For example the constructor could be private.

This situation occurs in several places in the SYCL spec. We want to document that one of the implicitly-defined member functions is not available to the application. We currently shows these cases as = delete in the code synopsis. You are correct, though, that an implementation could make these member functions unavailable in other ways, though. How does the C++ specification handle cases like this? Is there an example of a standard C++ class whose default constructor is not available? How does the C++ specification illustrate this?

There is a similar situation with std::atomic_ref, where the copy-assignment operator is not available. The C++ specification documents the operator as = delete. Does this mean that an implementation must actually delete the operator, or could an implementation make the operator unavailable in some other way (such as declaring it private)?

@Pennycook
Copy link
Contributor

I agree with Ronan here. We need to be precise about what "not user constructible" means, and I think it's different to saying that the default constructor is deleted.

I ran some experiments (see https://godbolt.org/z/d1h4o8Ge3), and the results are a little surprising. clang doesn't complain about the construction of b below in C++17, but does in C++20.

struct S2 {
    S2() = delete;
};
S2 a; // this is invalid
S2 b{}; // this is (surprisingly) valid, even though the constructor is deleted

struct S3 {
    private:
    S3() {};
};
S3 c; // this is invalid
S3 d{}; // this is also invalid

Does anybody understand this behavior?

Regardless, I think that in order to accept = delete as the solution here, we need to convince ourselves: 1) that it is sufficient to prevent users from constructing group objects; and 2) that we want to forbid implementations from defining private default constructors for group objects.

@gmlueck gmlueck removed the editorial Some purely editorial problem label Sep 27, 2024
@nliber
Copy link
Collaborator

nliber commented Sep 27, 2024

@Pennycook There was a core issue on this (I can't check at the moment because open-std.org is down).

The reason it compiles in C++17 is because S2 b{}; is doing aggregate initialization and ignoring the default constructor. This got fixed in C++20.

@Pennycook
Copy link
Contributor

@Pennycook There was a core issue on this (I can't check at the moment because open-std.org is down).

The reason it compiles in C++17 is because S2 b{}; is doing aggregate initialization and ignoring the default constructor. This got fixed in C++20.

Oof. Does that mean that we can't use = delete to specify this in SYCL 2020, but we could in a future version of SYCL (if it adopted C++20 as the minimum language version)?

@nliber
Copy link
Collaborator

nliber commented Sep 27, 2024

I think we can use = delete;. None of the real types are actually aggregates, are they? Specifically, while the definition of aggregate has changed over the years, anything with at least one non-public non-static data member has never been an aggregate.

We might have to call out that these types are not aggregates.

@Pennycook
Copy link
Contributor

None of the real types are actually aggregates, are they?

I don't think they're required to be, but I think they currently can be.

After the discussion in #532, we reached the conclusion that classes like nd_item and group could legally be stateless (i.e., they don't need to store the local range, etc, but can query it directly with an intrinsic). The DPC++ implementation of nd_item is now stateless and has no data members. I might be wrong, but I think it would be an aggregate if not for the fact that it declares a protected default constructor as an implementation detail.

We might have to call out that these types are not aggregates.

Would we just say something like "These types must not be aggregates." and leave it up to individual implementations to decide how to enforce that? I can imagine some implementations will want to rely on private/protected members, while others will want to rely on a private/protected constructor.

@keryell
Copy link
Member

keryell commented Sep 30, 2024

After the discussion in #532, we reached the conclusion that classes like nd_item and group could legally be stateless (i.e., they don't need to store the local range, etc, but can query it directly with an intrinsic). The DPC++ implementation of nd_item is now stateless and has no data members. I might be wrong, but I think it would be an aggregate if not for the fact that it declares a protected default constructor as an implementation detail.

Stateless means empty, which does not seem like an aggregate to me, since in C++ it will have a size of 1 anyway to enforce uniqueness of objects.

Otherwise, I was looking at some types in C++. For example https://en.cppreference.com/w/cpp/types/type_info describes the constructor as deleted but looking more carefully at https://eel.is/c++draft/type.info it is just that at least there is no exposed public constructor.

At the end, this is not super important. We could always have a public constructor = delete and have some other private constructor with some tag-dispatch.

@nliber
Copy link
Collaborator

nliber commented Sep 30, 2024

> Would we just say something like "These types must not be aggregates." and leave it up to individual implementations to decide how to enforce that?

I think so, especially since this is really just an issue for those compilers supporting C++17. Any version of SYCL that requires C++20 or later will likely just delete the default constructor.

@AlexeySachkov
Copy link
Contributor Author

Just curious about this implementation detail leakage: I have the feeling that "not user-constructible" does not mean it has no default constructor, just it cannot be constructed by the user. For example the constructor could be private.

Looking into 16.3.2.4 Detailed specifications it says the following:

Descriptions of class member functions follow the order (as appropriate): 138
...
138) To save space, items that do not apply to a class are omitted. For example, if a class does not specify any comparison operator functions, there will be no “Comparison operator functions” subclause.

This makes me think that we should actually omit the description of any constructors to say that there aren't any (user-visible) constructors.

@keryell
Copy link
Member

keryell commented Oct 3, 2024

There is a similar situation with std::atomic_ref, where the copy-assignment operator is not available. The C++ specification documents the operator as = delete. Does this mean that an implementation must actually delete the operator, or could an implementation make the operator unavailable in some other way (such as declaring it private)?

std::atomic_ref is very different because it defines a reference wrapper, so a default constructor does not make sense, even internally.

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