Skip to content

Remove redundant attributes#2272

Merged
monarchdodra merged 1 commit intodlang:masterfrom
9rnsr:refactor_parse
Jun 27, 2014
Merged

Remove redundant attributes#2272
monarchdodra merged 1 commit intodlang:masterfrom
9rnsr:refactor_parse

Conversation

@9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Jun 26, 2014

Required by: dlang/dmd#3696

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't this meant to be inout(T)* ? Your change seems to have flagged an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know the intention of first author, but currently @property inout T* peek(T)() inout is exactly same with @property T* peek(T)() inout. So this PR changes nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that, the return statements in function body now return T*.

Copy link
Member

Choose a reason for hiding this comment

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

Those returns ALSO have casts. It's pretty obvious that the casts should be removed. An immutable or const Variant should not return a mutable reference to itself.

This incorrectly compiles and runs without error with the existing compiler, and likely with your changes as well:

import std.variant;

void main()
{
    immutable v = Variant(5);
    assert(v == 5);
    auto p = v.peek!int();
    assert(p);
    *p = 4;
    assert(v == 4);
}

Note: Variant really has no good support for being const or immutable, so I'm not even sure why it has any inout support anyway. I couldn't even print or get the value except (ironically) via peek.

@monarchdodra
Copy link
Collaborator

OK, so it seems the code used to (incorrectly) compile because there was a cast. If I'm not mistaken, the code can be fixed, by simply removing the cast, and returning the correct inout type.

@9rnsr : Would you mind making the fix? I realize you just wanted 3696 to pass, but (AFAIK) the intent of 3696 was specifically to catch these kinds of issues...

@schveiguy
Copy link
Member

I'm not sure it's relevant to this PR, at least for std.variant.Variant. It looks like the type is quite unusable as const or immutable, not sure why the peek function was annotated as inout.

I'm inclined to say, let's pull this, and fix the annotation issues in a separate bug/pull.

Edit, not annotation issues, but the cast issues.

@monarchdodra
Copy link
Collaborator

I'm inclined to say, let's pull this, and fix the annotation issues in a separate bug/pull.

Alright, works for me. Let's just not forget about them then.

monarchdodra added a commit that referenced this pull request Jun 27, 2014
@monarchdodra monarchdodra merged commit bbf0947 into dlang:master Jun 27, 2014
@schveiguy
Copy link
Member

Issue 13000

@9rnsr 9rnsr deleted the refactor_parse branch June 28, 2014 02:51
@9rnsr
Copy link
Contributor Author

9rnsr commented Jun 28, 2014

Thanks for merging. and I confirmed the issue. To fix it, I opened #2279.

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.

3 participants