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

Document recent -dip1000 changes #2453

Merged
merged 1 commit into from
Jan 5, 2019
Merged

Conversation

thewilsonator
Copy link
Contributor

Document recent dip1000 related changes in:
dlang/dmd#8346
dlang/dmd#8408
dlang/dmd#8504

@WalterBright is this an accurate description?

cc @JinShil

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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.

@JinShil
Copy link
Contributor

JinShil commented Aug 23, 2018

This is great, but I think it needs to be more formally described in the spec:

dlang/dmd#8346

That PR causes the compiler to silently remove the return storage class under certain conditions. I'd like that documented at https://dlang.org/spec/function.html#return-ref-parameters

Furthermore the return storage class is not yet documented in the table at https://dlang.org/spec/function.html#param-storage

dlang/dmd#8408

I think this PR should be reverted, and we should come up with another way for the programmer to meta-programatically infer the storage classes.

But, since that's unlikely, I'd like for the inference rules to documented in the spec. I'd also like for the spec to make it clear when inference takes place -- I think it only takes place for templates, not for ordinary functions or methods. That needs to clarified.

Also, what is maybeScope and how does that work. Maybe you can distill the following into something comprehendible.
https://github.com/dlang/dmd/blob/74f89cc8f876279093c3dd6ef9a3918db329db73/src/dmd/escape.d#L1601-L1620

dlang/dmd#8504

I think this requires mention in https://dlang.org/spec/function.html#return-ref-parameters. 1) The first parameter is special, iff it's ref or out (I think). 2) special mention for constructors that the first parameter is essentially a ref this. But I'm still not sure how this works for member functions as those too have an implicit this. 3) The relationship between return parameters and the special first ref parameter.

You're not going to buy me off with a summary and a few paragraphs 😛. I think there's more to it than that. We also have to consider what this does to the DIP 1000 document. Personally, I think before any of these PR were/are merged there should be a PR to DIP1000 approving the design first.

@JinShil
Copy link
Contributor

JinShil commented Aug 23, 2018

There's also a lot in https://wiki.dlang.org/DIP25 and https://github.com/dlang/DIPs/blob/master/DIPs/DIP1000.md that should be folded into the spec with multiple examples illustrating the concepts.

@JinShil
Copy link
Contributor

JinShil commented Aug 23, 2018

Another thing to consider is that a lot of these features are feature-gated behind -dip25 and -dip1000 command-line switches. I'm not sure about the status of DIP 25 and DIP 1000. Are they draft, experimental, transitional, or something-else features? Perhaps they should be put in their own page in the spec and only folded into the spec if/when they become a permanent part of the language.

@thewilsonator
Copy link
Contributor Author

You're not going to buy me off with a summary and a few paragraphs

I wasn't expecting to. This is also for me to understand the state of the implementation and verify that it is what I think it is, which is probably not true in the general case.

  1. The first parameter is special, iff it's ref or out

With dlang/dmd#8346 it's ignored if its not a pointer (incl. ref/out), so I think not?

Also, what is maybeScope and how does that work. Maybe you can distill the following into something comprehendible.

No idea either.

@ntrel
Copy link
Contributor

ntrel commented Aug 23, 2018

@JinShil:

a lot of these features are feature-gated behind -dip25 and -dip1000 command-line switches

Return ref is already documented with a note about needing -dip25. I've added a note about -dip1000 for scope in #2452 #2454.

@JinShil
Copy link
Contributor

JinShil commented Aug 23, 2018

Return ref is already documented with a note about needing -dip25. I've added a note about -dip1000 for scope in #2452.

Thank you. But I still recommend moving anything related to DIP 25 and 1000 to a separate page until the design and implementation are finalized.

@ntrel
Copy link
Contributor

ntrel commented Aug 24, 2018

@thewilsonator I've made some changes here to improve wording (and formatting). (Unfortunately github won't let me set your repo as the base for a PR for some reason).

edit: See the full commit message for details.

@thewilsonator
Copy link
Contributor Author

Thanks will merge them tomorrow.

@thewilsonator
Copy link
Contributor Author

@ntrel Applied with some minor edits.

@thewilsonator
Copy link
Contributor Author

Pinging @WalterBright again.

@PetarKirov
Copy link
Member

@andralex can you review this?


$(P The function parameter attributes $(D return) and $(D scope) are used to track what happens to low-level pointers
passed to functions. Such pointers include: pointers, $(D this), classes, `ref` parameters, delegate/lazy parameters,
and aggregates containing a pointer.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Arrays? Or do you count those as aggregates?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest we use raw pointers, this, variables of reference type, ref/lazy parameters, ref return values and aggregates containing such pointers.

@thewilsonator
Copy link
Contributor Author

Pinging @WalterBright again: if you want me to be a functional PR manager, you need to review this.

@12345swordy
Copy link

@thewilsonator Try emailing him directly if he is not responding.

spec/memory-safe-d.dd Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

Woohoooo! Now I just need to figure out how to fix the LaTeX errors...

spec/memory-safe-d.dd Outdated Show resolved Hide resolved
spec/memory-safe-d.dd Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

Hmm, can't tell if that actually improved things. I'm going to split this PR.

@wilzbach
Copy link
Member

wilzbach commented Jan 4, 2019

Why don't you build it locally?

spec/function.dd Outdated
@@ -1241,8 +1241,13 @@ int foo(in int x, out int y, ref int z, int q);
$(TROW $(D scope), references in the parameter
cannot be escaped (e.g. assigned to a global variable).
Ignored for parameters with no references)
$(TROW $(D return), Parameter may be returned or copied to the first parameter,
Copy link
Member

Choose a reason for hiding this comment

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

The LaTeX errors are caused by the last comma in this line. The comma is interpreted by ddoc as a parameter separator, which in LaTeX is translated into an column separator (ampersand) because you're in a table. To fix, you may want to use $(TROW $(D return), $(ARGS Parameter may be returned...))

@andralex
Copy link
Member

andralex commented Jan 4, 2019

Made a commit directly in the interest of time.

@thewilsonator
Copy link
Contributor Author

Thanks, I fixed some whitespace you left. Hopefully this works.

@thewilsonator
Copy link
Contributor Author

IT WORKS!!! Thanks everyone!

@WalterBright
Copy link
Member

@tgehr I cannot see a rationale here for this change.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 16, 2021

Probably just an oversight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking other work Spec / content Vision https://wiki.dlang.org/Vision/2018H1
Projects
None yet
Development

Successfully merging this pull request may close these issues.