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

fix Issue 15869 - RVO can overwrite argument #8200

Merged
merged 1 commit into from
May 14, 2018

Conversation

WalterBright
Copy link
Member

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
15869 normal RVO can overwrite argument

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#8200"

{
foreach (i, f; ad.fields)
{
if (f == vd)
Copy link
Member

Choose a reason for hiding this comment

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

If (f != vd) continue? As its quite a nest you have here of statements.

/* If the address of vd is taken, assume it is thereby initialized
* https://issues.dlang.org/show_bug.cgi?id=15869
*/
modifyFieldVar(loc, sc, vd, e1);
Copy link
Contributor

@RazvanN7 RazvanN7 Apr 23, 2018

Choose a reason for hiding this comment

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

what if the value isn't actually initialized? Maybe it should be checked that vd._init !is null otherwise issue an error that the address cannot be taken if the variable isn't initialized and the default constructor is disabled

Copy link
Member Author

Choose a reason for hiding this comment

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

It not being initialized should only be possible in @System code, where anything goes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it odd that the compiler forces you to initialize the field but lets you use it before initialization. I agree that if the struct does have a default constructor it should be allowed, but when it's missing why am I forced to initialize it if I can use the struct anyway?
For example, this code compiles:

struct A
{
    @disable this();
    this(string) {}
    int c;
}

struct B
{
    A a;
    int c;
    this(int)
    {   
        c = a.c; 
        a = A("string"); // if this is commented, error : field `a` must be initialized in constructor
       // why? if I can use the fields anyway...
    }   
}

void main() {}

Copy link
Member Author

Choose a reason for hiding this comment

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

By having a constructor, and default construction disabled, you have to initialize it explicitly rather than it being default initialized. It's the whole point of disabling default construction. The initial value, however, is still defined and is still useful.

@ibuclaw ibuclaw self-assigned this May 2, 2018
@dlang-bot dlang-bot merged commit 68eb9d3 into dlang:master May 14, 2018
@WalterBright WalterBright deleted the fix15869 branch May 14, 2018 12:27
{
if (!(sc.ctorflow.fieldinit[i] & CSX.this_ctor))
{
/* If the address of vd is taken, assume it is thereby initialized
Copy link
Contributor

@RazvanN7 RazvanN7 May 17, 2018

Choose a reason for hiding this comment

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

This needs to be documented somewhere, otherwise you may end up with surprising error messages. For example, this code:

immutable(int)* g;

struct X { 
    int a = 10; 
    immutable this(int x)
    {   
            g = &a; 
            a = 42;    // error
    }   
}

void main()
{
    auto x = immutable X();
}

Leads to : test.d(7): Error: immutable field a initialized multiple times which is correct but not obvious

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should point to the first initialization in the error message. And maybe a special message explaining why taking address is initialization.

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