Skip to content

Revert "remove optabgen.d"#10238

Closed
Geod24 wants to merge 1 commit intomasterfrom
revert-10221-optabgen
Closed

Revert "remove optabgen.d"#10238
Geod24 wants to merge 1 commit intomasterfrom
revert-10221-optabgen

Conversation

@Geod24
Copy link
Member

@Geod24 Geod24 commented Jul 29, 2019

Reverts #10221

Let's see what SemaphoreCI says.
(I wish Github opened revert branches in forks...)

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

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

@marler8997
Copy link
Contributor

What's the reason for the revert?

@rainers
Copy link
Member

rainers commented Jul 29, 2019

What's the reason for the revert?

master failing to build on semaphoreci: https://semaphoreci.com/dlang/dmd-2/branches/master

@Geod24
Copy link
Member Author

Geod24 commented Jul 29, 2019

The PR broke SemaphoreCI. Master is currently red: https://semaphoreci.com/dlang/dmd-2
But this PR fixes it.
CC @thewilsonator

@marler8997
Copy link
Contributor

Shoot, now I'm going to have to rebase #10217 after already rebasing it from #10221. Then if this gets fixed and re-merged, then I"ll have to rebase/rework again. Is it possible to try to fix the failure before reverting?

@Geod24
Copy link
Member Author

Geod24 commented Jul 29, 2019

You can just use git reflog (and Github might still show the commits).
In any case, having the reverter fix the bug is a bad trend: it shift the burden of work to the person that finds the bug, not the one that creates the bug.

@marler8997
Copy link
Contributor

marler8997 commented Jul 29, 2019

You can just use git reflog (and Github might still show the commits).

Sure but I've made changes since the rebase so my original commit is no longer valid. It's not a big deal, I'm just letting you know that this is causing extra work.

In any case, having the reverter fix the bug is a bad trend: it shift the burden of work to the person that finds the bug, not the one that creates the bug.

I'm not worried about trends. You have every right to revert the commit, but that doesn't mean we can't try to fix it first either. You don't have a responsibility to fix it of course, I was just asking if it could be easily done. If not then by all means go ahead with the revert (not that you need my permission or anything).

@marler8997
Copy link
Contributor

Ping @WalterBright, do you want a chance to come up with a quick fix before the change is reverted? Or should we go ahead with the revert now and fix this later?

@marler8997
Copy link
Contributor

marler8997 commented Jul 29, 2019

Interesting, semaphore is currently passing with my PR, which has been rebased with the "remove optabgen": https://semaphoreci.com/dlang/dmd-2/branches/pull-request-10212/builds/22

@marler8997
Copy link
Contributor

Well I couldn't figure out the cause in the few minutes I spent, so don't hold back on my account. I'm off to bed.

@rainers
Copy link
Member

rainers commented Jul 29, 2019

I don't see why removing an usused generator can produce different binaries, but I suspect we hit some threshold producing different code due to something like

if (go.changes && go.mfoptim & MFloop && (os_clock() - starttime) < 30 * CLOCKS_PER_SEC)

in the optimizer.

@rainers
Copy link
Member

rainers commented Jul 29, 2019

Maybe #10240 helps.

@rainers
Copy link
Member

rainers commented Jul 30, 2019

Maybe #10240 helps.

it didn't but #10242 seems to.

@Geod24
Copy link
Member Author

Geod24 commented Jul 30, 2019

Thanks @rainers !

@Geod24 Geod24 closed this Jul 30, 2019
@Geod24 Geod24 deleted the revert-10221-optabgen branch July 30, 2019 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants