Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Jul 15, 2019

In support of dlang/dmd#10179

Followup to #2676 and dlang/phobos#7110

in as a parameter storage class is defined as scope const. However in has not yet
been properly implemented so its current implementation is equivalent to const. Properly
implementing in now will likely break code, so it is recommended to avoid using in, and
explicitly use const or scope const instead, until in is properly implemented.

The use of in as a parameter storage class is already discouraged in the documentation. See https://dlang.org/spec/function.html#parameters

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @JinShil! 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

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 + druntime#2677"

@JinShil
Copy link
Contributor Author

JinShil commented Jul 15, 2019

I've replaced in with const scope in D code, but replaced in with scope for C bindings, since C doesn't have any concept of scoped pointers.

@JinShil
Copy link
Contributor Author

JinShil commented Jul 15, 2019

I think I need to split this up into multiple PRs.

@jmdavis
Copy link
Member

jmdavis commented Jul 15, 2019

Not only should this be multiple PRs, but we shouldn't be blindly changing in to const scope. As it is, it looks like a bunch of instances of in in std.datetime were changed to const scope already, and that will likely break existing code when it switches on -dip1000, because the code uses explicitly typed delegates, and they unfortunately used in (really, nothing in std.datetime should have used in, but when I originally wrote it, I didn't understand in properly and somehow had gotten the idea that it was best practice, and while some of that could be fixed now, not all of it really can be). Given that in has never actually meant const scope, it would be far more correct to simply change all instances of in to const than to change them to const scope, though changing them from in to anything risks code breakage when explicit function or delegate types are involved.

So, any changes of in to anything need to be examined carefully to see whether scope makes any sense and whether using scope risks breaking code. I seriously doubt that you've done that with every instance of in in all 110 files in this PR, and there isn't much chance that a reviewer would manage to either. Such changes should only be done in relatively small batches where it's actually reasonable to examine each change in detail. And since all of the CI stuff is failing, presumably, the changes here don't even pass the test suite, but even if they did, that doesn't necessarily mean that they wouldn't be breaking existing code, particularly since it's not like we have extensive testing for whether the code in question works properly with scope. It would be easy to have code that worked just fine with tests using built-in types or simple user-defined types but which failed miserably with more complex types if scope were applied with -dip1000.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 2, 2019

A decision about in has finally been made: dlang/dmd#10382 (comment)

@JinShil JinShil closed this Sep 2, 2019
@JinShil JinShil deleted the discourage_in_2 branch September 2, 2019 09:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants