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 20653 . Short circuit manifest constant evaluation #10939

Merged
merged 3 commits into from
Mar 19, 2020

Conversation

benjones
Copy link
Contributor

The fix is adding the SCOPE.condition flag when doing CTFE to infer the VarDecl type in dsymbolsem.

The rest of the changes are fixes to the debug logging. I can remove them if they're distracting

@benjones benjones marked this pull request as ready for review March 18, 2020 02:24
@WalterBright
Copy link
Member

Please explain the point of this. Is it a bug fix? An enchancement?

@thewilsonator
Copy link
Contributor

Is it a bug fix?

fix issue 20653

Dunno where the bot is though.

@thewilsonator
Copy link
Contributor

The rest of the changes are fixes to the debug logging. I can remove them if they're distracting

Can you split them into a different commit?

@benjones
Copy link
Contributor Author

I started this as a draft PR, so maybe @dlang-bot didn't notice when I dropped the draft.

I'll split into 2 commits. I'll also add a test with &&

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @benjones! 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 coverage diff by visiting the details link of the codecov check)
  • 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

Auto-close Bugzilla Severity Description
20653 normal Short-circuiting boolean logic not working

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#10939"

@Geod24
Copy link
Member

Geod24 commented Mar 18, 2020

Could you add a test with a template instantiation ?
Something like:

T myFunc() { return T.init; }
enum Foo(T) = is(T == int) || myFunc!T();

void main ()
{
    assert(myFunc!int() == 0);
}

The idea is to check that myFunc!int is properly emitted and does not cause linking error.

@TurkeyMan
Copy link
Contributor

Dunno where the bot is though.

I mean, cheeky bugger's probably self-isolating!

@WalterBright
Copy link
Member

cheeky bugger's

I can't let you Aussies have a monopoly on cool phrases. It's not fair!

@TurkeyMan
Copy link
Contributor

😛

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Mar 18, 2020

Could you add a test with a template instantiation ?
Something like: [...]
The idea is to check that myFunc!int is properly emitted and does not cause linking error.

Could also add this to the compilable test and force linking with

// LINK:

@@ -805,9 +806,11 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor
// so mark the scope as ctfe if required
bool needctfe = (dsym.storage_class & (STC.manifest | STC.static_)) != 0;
if (needctfe)
{
sc.flags |= SCOPE.condition;
Copy link
Contributor

@ntrel ntrel Apr 5, 2023

Choose a reason for hiding this comment

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

@benjones This breaks the error for is Identifier forms used outside of static if/assert:
https://issues.dlang.org/show_bug.cgi?id=23827

I tried making a fix but it doesn't pass the test suite.

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 saw your new PR. It's been so long, I don't remember how I came to think this was the right fix and probably won't have time to look closely in the next couple of weeks.

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.

8 participants