Skip to content

Conversation

@WalterBright
Copy link
Member

fix https://issues.dlang.org/show_bug.cgi?id=519

Will follow up with change to spec.

@WalterBright
Copy link
Member Author

Documentation fix: dlang/dlang.org#699

@WalterBright WalterBright force-pushed the fix519 branch 2 times, most recently from 5732e62 to 8174c5e Compare November 12, 2014 06:05
@WalterBright
Copy link
Member Author

Phew! finally passing.

src/class.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add disabled code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to leave it in until the controversy is over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean leave it in the pull request or leave it in the compiler's source?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes and yes until the controversy is over.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Controversy is over" – where does the actual discussion take place? The last related reply in the bugzilla issue was in 2012.

Copy link
Member Author

Choose a reason for hiding this comment

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

Part of my solution is to change the spec to say the invariant won't run for trivial construction. Since obviously several people expect it to, I await their response.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like we should have that discussion before making the change.

Copy link
Member

Choose a reason for hiding this comment

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

@WalterBright
Copy link
Member Author

Sigh. I cannot reproduce these failures in std.datetime:

../dmd/src/dmd -O -w -L/co -c -unittest -ofunittest3b.obj std\datetime.d
std\datetime.d(17339): Error: function std.datetime.Interval!(Date).Interval.end need 'this' to access member end
std\datetime.d(17372):        called from here: end()
std\datetime.d(18694): Error: template instance std.datetime.Interval!(Date) error instantiating

Anyone know what is going on?

@WalterBright
Copy link
Member Author

Figured it out. Needs this phobos pull:

dlang/phobos#2689

@yebblies
Copy link
Contributor

Any idea why it worked before?

@WalterBright
Copy link
Member Author

I don't know how datetime works. It's got too many layers to it :-(

src/struct.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in comment.

@MartinNowak
Copy link
Member

I don't know how datetime works. It's got too many layers to it :-(

Can we actually please figure out why you had to rewrite legit code.
You can use dustmite to reduce test-cases.

The problem only appeared on Win which is quite weird when you look at your phobos pull.
Also now there is a new Win32 issue (https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=1256128&isPull=true).

@WalterBright
Copy link
Member Author

Also now there is a new Win32 issue (https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=1256128&isPull=true).

This can be reproduced by adding the line:

public pure nothrow @safe @nogc ~this() { }

to struct Date and running with the master dmd. It's some other problem.

@MartinNowak
Copy link
Member

This error looks a bit like the pull messed up trySemantic, just a guess though.

@WalterBright
Copy link
Member Author

@MartinNowak
Copy link
Member

Any objections against this change in general?

@MartinNowak
Copy link
Member

@WalterBright it would be fairly interesting to get your opinion on this.
http://forum.dlang.org/post/m4eu6v$trq$1@digitalmars.com

@MartinNowak
Copy link
Member

I'm uneasy with this. Why would you need to check the invariant before destroying structs/classes that don't have a destructor? No code could ever use that value again.
Before this change one could leave back such a value in any state, now I might have to do additional cleanup before it is destroyed.

@MartinNowak
Copy link
Member

Ping, @WalterBright what do you think?

@WalterBright
Copy link
Member Author

Why would you need to check the invariant before destroying structs/classes that don't have a destructor?

Same as always, to check for bugs. Live objects should always be in a valid state, even if they are headed for the gallows.

@WalterBright
Copy link
Member Author

what do you think?

I think this should be pulled.

@WalterBright
Copy link
Member Author

it would be fairly interesting to get your opinion on this.

I replied in the n.g.

@andralex
Copy link
Member

@WalterBright we should pull this. Rebase?

@WalterBright
Copy link
Member Author

rebased for ddmd

@andralex
Copy link
Member

andralex commented Jan 3, 2017

@WalterBright @MartinNowak status?

@andralex
Copy link
Member

andralex commented Jan 8, 2017

Why would you need to check the invariant before destroying structs/classes that don't have a destructor? No code could ever use that value again.

Walter is right - object should be valid for destruction

@andralex
Copy link
Member

andralex commented Jan 8, 2017

@WalterBright need rebase

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
519 Invariant not called from autogenerated class/struct constructor/destructor

@andralex
Copy link
Member

andralex commented Jan 8, 2017

legit errors in autotester

@andralex
Copy link
Member

ping @WalterBright

@ibuclaw
Copy link
Member

ibuclaw commented Dec 27, 2017

Rebased patch in #7536.

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.

7 participants