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

rewrite do-while loop as for loop #16187

Closed
wants to merge 2 commits into from

Conversation

WalterBright
Copy link
Member

I should have thought of this a long time ago.

@WalterBright WalterBright added WIP Work In Progress - not ready for review or pulling Easy Review Refactoring No semantic changes to code labels Feb 14, 2024
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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 run digger -- build "master + dmd#16187"

@WalterBright
Copy link
Member Author

This baffles me:

++ diff -up --strip-trailing-cr runnable/extra-files/runnable-cov2.lst 'D:\a\dmd\dmd\compiler\test\test_results/runnable/runnable-cov2.lst2'

--- runnable/extra-files/runnable-cov2.lst	2024-02-14 23:48:55.360386500 +0000

+++ "D:\\a\\dmd\\dmd\\compiler\\test\\test_results/runnable/runnable-cov2.lst2"	2024-02-14 23:52:09.81466[200](https://github.com/dlang/dmd/actions/runs/7908873893/job/21588901790?pr=16187#step:10:201)0 +0000

@@ -71,4 +71,3 @@

        1|    test24264();

        1|    return 0;

         |}

-cov2.d is 95% covered

The generated .lst file and the expected .lst file are identical.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 15, 2024

The generated .lst file and the expected .lst file are identical.

Remove the last line.

-cov2.d is 95% covered

@ibuclaw
Copy link
Member

ibuclaw commented Feb 15, 2024

I'm expecting 100 or so lines to be removed then from blockexit, dinterpret, toir, ...

@WalterBright
Copy link
Member Author

@ibuclaw I am, too, I'm just waiting for it to work before taking a meat-axe to it.

@WalterBright
Copy link
Member Author

I'm not getting the fail220.d or the ice17074.d extra error messages on my setup. Neither of those files has a do-while in it, either.

@WalterBright
Copy link
Member Author

Remove the last line.

Thanks, that worked

@WalterBright
Copy link
Member Author

The 40 minute timeouts suggest an infinite loop in the compiler.

@WalterBright
Copy link
Member Author

Which means I need to replace every do-while in the compiler with a while, till I find out which one is broken.

@braddr
Copy link
Member

braddr commented Feb 15, 2024

A counter and a begin/end range to control when to do the old vs new behavior would allow skipping the manual code updating and allow for binary searching the codebase.

@WalterBright
Copy link
Member Author

Hi Brad! Nice to see you around.

The real time waster is waiting 40 minutes for the test suite to run for each iteration. There are only a handful of do-while's in the source code, so the manual edits aren't that bad. The culprit may be in druntime, too.

I must have done something stupid with this simple change, but I'm blind to what it is. Grump.

@WalterBright
Copy link
Member Author

I think I'll try your idea...

@WalterBright
Copy link
Member Author

Definitely frustrating. I removed all the compiler uses of 'do' but the problem remains.

@tim-dlang
Copy link
Contributor

The rewrite is not equivalent for do-while loops containing continue. Consider this example:

import std.stdio;
void main()
{
    int i = 10;
    do
    {
        writeln(i);
        continue;
    }
    while(--i > 0);
}

Previously this printed the numbers from 10 to 1, but with this PR it is an endless loop always printing 10.

@WalterBright
Copy link
Member Author

@tim-dlang wow, thanks! How did you figure that out?

@tim-dlang
Copy link
Contributor

@tim-dlang wow, thanks! How did you figure that out?

Sometime ago I also wondered, why the do-while loop is not lowered like other loops.

@WalterBright
Copy link
Member Author

I'll have to think of another way to lower it!

@WalterBright
Copy link
Member Author

The way to make this work is to have the continue point to the if statement. Unfortunately, the continue logic in the compiler is overly complex. Figuring out how to patch it is not a net improvement over just having a do-while construct.

I'm going to reluctantly close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Review Refactoring No semantic changes to code WIP Work In Progress - not ready for review or pulling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants