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 19285 - False GC positive caused by AddressOf inside typeof… #14195

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

maxhaton
Copy link
Member

@maxhaton maxhaton commented Jun 8, 2022

… polluting global semantic state.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @maxhaton!

Bugzilla references

Auto-close Bugzilla Severity Description
19285 major false positive GC inferred

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 + dmd#14195"

@thewilsonator
Copy link
Contributor

Test case?

@maxhaton
Copy link
Member Author

maxhaton commented Jun 9, 2022

Test case?

added

@BorisCarvajal
Copy link
Member

What about?

enum Unused3 = __traits(compiles , &inner);

@maxhaton
Copy link
Member Author

maxhaton commented Jun 9, 2022

What about?

enum Unused3 = __traits(compiles , &inner);

Fixed and added

… polluting global semantic state.

This patch also adds a new method isFromSpeculativeSemanticContext
to the Scope struct to avoid repition of this logic in the compiler.
*
* Returns: `true` if this `Scope` is known to be from one of these speculative contexts
*/
extern(C++) bool isFromSpeculativeSemanticContext() scope
Copy link
Member

Choose a reason for hiding this comment

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

A detail but the name is really long, why not isSpeculative?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why C++? Does ldc use this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not C++? The second part I cannot answer causally

Copy link
Member

Choose a reason for hiding this comment

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

Keep internal semantic machinery internal.

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 isn't machinery as per se, this could be useful for culling emitted symbols or more weakly a speculative symbol may still need to be emitted into the binary but doesn't need to be optimized because it'll never be called.

Copy link
Member

Choose a reason for hiding this comment

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

That's still internal to the front-end (see dmd.dtemplate.needsCodegen).

@thewilsonator thewilsonator merged commit 20b004f into dlang:master Jun 9, 2022
Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

.

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