Skip to content

Conversation

@pbackus
Copy link
Contributor

@pbackus pbackus commented Mar 19, 2021

…e index

See pbackus/sumtype#57.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @pbackus! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21731 enhancement SumType should provide convenient access to the type index

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7886"

{
alias MySum = SumType!(const(int[]), int[]);

int[] ma = [1, 2, 3];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int[] ma = [1, 2, 3];
int[3] ma = [1, 2, 3];

Wouldn't that be enough to make it work in betterC ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, yes. In practice, if I use a stack-allocated static array and construct the SumType with a slice (i.e., MySum(ma[])), the tests will fail even with -preview=dip1000, because the workaround for issue 21229 used in SumType's constructors does not correctly preserve lifetime information about the constructor's argument.

@schveiguy
Copy link
Member

Why isn't this just return tag? Doesn't that do the same thing?

@pbackus pbackus marked this pull request as draft March 19, 2021 17:48
@pbackus
Copy link
Contributor Author

pbackus commented Mar 19, 2021

In some rare cases (see unittests) it is possible for the same type to have two different tag values. The current implementation always returns a unique value for a given type, but conversation at pbackus/sumtype#57 has convinced me that this may not be desirable.

@schveiguy
Copy link
Member

Hm... so in a case where you have SumType!(int, const int), assigned to a const int, the tag will be 1 with a standard instance, and 0 with a const instance?

I'll point out that returning a value of 1 is still going to be valid in the list of (const int, const int). I would recommend just returning the tag, it's the most expected result, even on a const SumType.

@pbackus
Copy link
Contributor Author

pbackus commented Mar 19, 2021

Yes, I've come the same conclusion myself.

@pbackus pbackus force-pushed the sumtype-type-index branch from af1f560 to 5ed6ffd Compare March 19, 2021 19:10
@pbackus pbackus marked this pull request as ready for review March 19, 2021 19:10
@pbackus
Copy link
Contributor Author

pbackus commented Mar 19, 2021

Buildkite failure appears to be spurious:

fork/exec /usr/bin/buildkite-agent: cannot allocate memory

@pbackus pbackus force-pushed the sumtype-type-index branch from 5ed6ffd to 50c7945 Compare March 19, 2021 21:12
Copy link
Member

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

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

Looks good. Trivial change, looks like some docs were left over from the previous incarnation.

std/sumtype.d Outdated
* `SumType`'s `Types`.
*
* If the `SumType` is qualified, then its qualifiers are applied to
* `Types` before determining the index.
Copy link
Member

Choose a reason for hiding this comment

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

This second paragraph should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it? In the case of something like const(SumType!(A, B))(const(A)()), the type of the value is const(A), which is not present in Types. The first paragraph alone is insufficient to determine the correct behavior in this case (although I'll grant that it is not difficult to guess).

Copy link
Member

Choose a reason for hiding this comment

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

Well, you aren't applying the qualifier. You are just returning the tag.

The paragraph suggests something different happens when a qualifier is used.

Perhaps a rewrite is needed. Maybe:

If the SumType is qualified, the index indicates the Type if it were unqualified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The specific wording you suggest is incorrect, because the type in Types might already be the qualified version, but I agree that a rewrite is the best solution here.

The new wording describes the result without implying anything about the
implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants