Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

@edi33416
Copy link
Contributor

@edi33416 edi33416 commented Mar 14, 2019

Struct dtors should always be called; we shouldn't allow configuration files to change the meaning of the language.

Edit: Anyone knows when and why was this introduced? Any projects using it?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @edi33416! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 + druntime#2509"

@thewilsonator
Copy link
Contributor

This i separate from the not calling the GC at all on shutdown, right?

@edi33416
Copy link
Contributor Author

This i separate from the not calling the GC at all on shutdown, right?

As far as I can tell, this is only used by structTypeInfoSize to determine if _d_newitem needs to store a ptr to the dtor. I don't think this is related with the GC shutdown.

@thewilsonator thewilsonator added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Mar 14, 2019
Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

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

This needs to go through the depreciation process.

@andralex
Copy link
Member

I wonder how to deprecate this - issue a message at runtime to stderr if the flag is false?

@Geod24
Copy link
Member

Geod24 commented Mar 15, 2019

Edit: Anyone knows when and why was this introduced? Any projects using it?

A long time ago, the GC would not call destructors on heap allocated struct. When it was fixed, I think this was put as a transitional step. #864

Copy link
Member

@rainers rainers left a comment

Choose a reason for hiding this comment

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

I'm fine with removal. AFAICT it has never been documented so we might consider it internal and skip a deprecation, or make that a short one.
Printing a deprecation message in lifetime_init if the setting is found seems like the obvious choice.

delete arr1;
assert(dtorCount == 7);

if (callStructDtorsDuringGC)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove scoping/indentation, too. Later it will just confuse people why it is there sometimes, and sometimes not.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Mar 15, 2019

I wonder how to deprecate this - issue a message at runtime to stderr if the flag is false?

I would print the message as long as the flag has been passed to the application, regardless of the value of the flag.

Regarding stderr. I don't know if we should worry about this, but on AppVeyor, if any output occurs on stderr the build is failed.

When the deprecation should turn into an error we can throw an exception.

@jacob-carlborg
Copy link
Contributor

AFAICT it has never been documented so we might consider it internal and skip a deprecation, or make that a short one.

Are any of the druntime flags documented?

@wilzbach
Copy link
Contributor

AFAICT it has never been documented so we might consider it internal and skip a deprecation, or make that a short one.

Are any of the druntime flags documented?

No. "Luckily", druntime is chronically under-documented.
The only documentation of --DRT is here: https://dlang.org/spec/garbage.html#gc_config (for the GC bits).

@jacob-carlborg
Copy link
Contributor

I think it should be deprecated anyway. It's not difficult to do. For me it's enough for it to be deprecated for one release.

@JohanEngelen
Copy link
Contributor

'// this is run before static ctors, so it is safe to modify immutables'

Wow, incredible. Very nice to get rid of this kind of UB in druntime code.

@dnadlinger
Copy link
Contributor

Wow, incredible. Very nice to get rid of this kind of UB in druntime code.

Immutable globals can be initialised in static constructors, so whether this is UB or not is questionable.

@andralex
Copy link
Member

@edi33416 ?

@edi33416
Copy link
Contributor Author

@andralex @jacob-carlborg I opened #2513 for the deprecation of the feature

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed stalled labels May 27, 2021
@RazvanN7 RazvanN7 closed this Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

72h no objection -> merge The PR will be merged if there are no objections raised. Needs Rebase needs a `git rebase` performed stalled

Projects

None yet

Development

Successfully merging this pull request may close these issues.