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

Revert "Remove backend globals from header modules" #10460

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

WalterBright
Copy link
Member

Reverts #10455, reason why is given there.

@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 fetch digger
dub run digger -- build "master + dmd#10460"

@marler8997
Copy link
Contributor

marler8997 commented Oct 8, 2019

I'm copying my comment from #10455:

Walter I opened #10255 3 months ago. In that original PR I had put these globals into a separate file so that the other modules would'nt have to import the entire cg87.d file. You left this comment:

Adding a new file seems like overkill for 3 variables.

So I left the variables in cg87.d made everything private except those variables and then imported cg87.d and waited for your feedback for 3 months. You were also pinged by @ZombineDev a month later and you still left no feedback.

In the future, I'm sure everyone would be very grateful if you did the following

  1. When you object to a change, and a developer refactors their code according to that objection, please follow up and respond.

  2. Instead of only saying why you don't like a change, consider providing guidance on how they could change their solution to a form that you will like.

That being said, I don't know how this change could affect the compilation time of the backend. The backend is compiled in a single invocation so the cg87.d file will only be parsed/analyzed once anyway. Do you have data that shows this increases compilation time? As @andralex says, "Always Measure".

@WalterBright
Copy link
Member Author

I also use the backend for the DMC++ compiler. Making changes just for DMD often break that, causing extra work for me, including this change. The backend uses the header/implementation file dichotomy, mixing that up here and there with the frontend style doesn't help much of any.

I'm sorry I had not noticed these changes earlier. There's only so much of me to go around. On the other hand, this is just a handful of lines, so it's not like a major effort was wasted.

I've repeatedly objected to making changes that are essentially shuffling files around. Finding declarations to make private, though, is a productive change.

@marler8997
Copy link
Contributor

I just don't want these internal backend globals to be in a shared header with the frontend. Do you have an alternative solution?

@WalterBright
Copy link
Member Author

You could make a backend.d header for the backend globals.

Copy link
Contributor

@marler8997 marler8997 left a comment

Choose a reason for hiding this comment

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

You could make a backend.d header for the backend globals.

Perfect. Go ahead and revert my original change and I'll work on that change.

@WalterBright WalterBright merged commit 6d0adf2 into master Oct 11, 2019
@WalterBright WalterBright deleted the revert-10455-backendEncapsulate2 branch October 11, 2019 05:56
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.

3 participants