Update struct.dd - postblit documentation #2281

RazvanN7 · 2018-03-19T09:44:19Z

No description provided.

dlang-bot · 2018-03-19T09:44:20Z

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

andralex · 2018-03-19T16:36:39Z

@RazvanN7 - need to define what happens when the entire struct is qualified and then defines a postblit with no qualifier/same qualifier/different qualifier.

andralex · 2018-03-19T16:37:42Z

@RazvanN7 please add examples after each major paragraphs. RUNNABLE_EXAMPLE should be great here. @wilzbach we'd really need a ERROR_EXAMPLE here, do you think you could add that to our build infra?

wilzbach

Self-containing + no information that will be out of date soon please

wilzbach · 2018-03-19T16:41:00Z

spec/struct.dd

+        $(D this(this) immutable;) or $(D this(this) shared;), then the code
+        is ill-formed. Historically, such definitions passed compilation but
+        could not be invoked. A deprecation diagnostic is issued by the front-end
+        of dmd 2.080 or later, see $(BUGZILLA 18417). Example:)


Don't add things that can get outdated into the spec, because they will.
Info this belongs into the changelog entry

wilzbach · 2018-03-19T16:42:13Z

spec/struct.dd

+
+        $(P If a postblit is qualified with `const` as in $(D this(this) const;),
+        the code is ill-formed. A deprecation diagnostic is issued by the
+        front-end of dmd 2.080 or later, see $(BUGZILLA 18417). Prior versions


Again information that will get outdated, better not add it.

wilzbach · 2018-03-19T16:43:46Z

spec/struct.dd

+        }
+        ---
+
+        $(P This allows incorrect code to compile, refer to $(BUGZILLA 18357) for examples.)


This doesn't work. The spec should be self-containing as Bugzilla might be offline or it simply might be print as ebook or PDF.

wilzbach · 2018-03-22T16:31:17Z

Out of interest: does this try to address https://issues.dlang.org/show_bug.cgi?id=14246?
(see also the proposed changes + tests in dlang/dmd#6816)

wilzbach · 2018-03-22T16:36:15Z

. @wilzbach we'd really need a ERROR_EXAMPLE here,

There's already SPEC_RUNNABLE_EXAMPLE_FAIL - doesn't that work for you?
I wanted to add support for the error messages too, but after thinking about it for a few weeks, I'm not so sure anymore. The problem is that every time when the error message gets updated, the spec would require an update too. Now the problem is how do you update the spec to the new compiler message without breaking DAutoTest? This would mean:

PR to dlang.org to disable the test
PR to dmd to fix the error message
PR to dlang.org to enable the test again and update the error message

Not sure whether we really want to increase our cross-repository even further.

RazvanN7 · 2018-03-22T17:22:30Z

Out of interest: does this try to address https://issues.dlang.org/show_bug.cgi?id=14246?

No, this PR documents solely the postblitting mechanism; it's a work in progress, though.

andralex

Couple of minor changes requested

andralex · 2018-03-22T18:02:21Z

spec/struct.dd

+        internal postblit functions:)
+
+        $(OL
+        $(LI `void __postblit()`. The compiler assigns this name to the explicitly


Please specify entire signatures (as I did in the commit I added) and make sure those I added are correct.

andralex · 2018-03-22T18:09:48Z

spec/struct.dd

+---
+)
+
+        $(P Neither of the above postblits is defined for structs that don't


Should specify somewhere (perhaps here) what happens if someone does NOT define this(this) but DOES define the special functions. Is the compiler going to insert the calls? (Hopefully not!)

andralex · 2018-03-22T18:36:39Z

spec/struct.dd

+---
+)
+
+        $(P Postblits are not overloadable. If two or more postblits are defined,


Better (missed this): "Postblits cannot be overloaded." The word "overloadable" is a bit awkward.

andralex · 2018-03-22T18:37:58Z

spec/struct.dd

+
+        $(OL
+        $(LI `const`. When a postblit is qualified with `const` as in
+        $(D this(this) const;) or $(D const this(this);) then the postblit


I removed the "is ill-formed" part here because the code is accepted. Please don't forget - we're documenting behavior here, not what should happen. If we deprecate this with the next version, then yes please do mention it's ill-formed but no diagnostic is issues by front-end versions prior to 2.xxx.

We will deprecate this with the next version, but @wilzbach argued that information that gets outdated should not be in the spec. I thought that saying it's ill-formed but not mentioning the deprecation is the best compromise since you can actually call the function but it's not doing what it is supposed to.

It's helpful to give that information so people who see invalid code getting compiled don't get confused.

andralex · 2018-03-22T21:14:17Z

spec/struct.dd

+        $(OL
+        $(LI `const`. When a postblit is qualified with `const` as in
+        $(D this(this) const;) or $(D const this(this);) then the postblit
+        is succesfully called on mutable (unqualified), `const`,


I removed the backquotes around mutable because mutable is not a keyword.

andralex · 2018-03-22T21:25:43Z

spec/struct.dd

+        $(LI `immutable`. When a postblit is qualified with `immutable`
+        as in $(D this(this) immutable) or $(D immutable this(this))
+        the code is ill-formed. The `immutable` postblit passes the
+        compilation phase but cannot be invoked. Example:)


Version when this will be diagnosed? I think we can leave this text as is and add a mention once we merge the appropriate PR.

wilzbach · 2018-03-30T18:29:15Z

Sorry for being late to the party, but should the behavior for an array of structs be defined too?
At the moment DMD calls __ArrayPostblit which is automatically added to object.d, but that might change soon: dlang/dmd#8067

In any case, the behavior for now is:

foreach (ref e; cast(S*)this.v.ptr)[0 .. n])
      e.__xpostblit();

RazvanN7 requested review from WalterBright and andralex as code owners March 19, 2018 09:44

wilzbach reviewed Mar 19, 2018

View reviewed changes

RazvanN7 force-pushed the patch-9 branch from 4895022 to a5066d3 Compare March 22, 2018 11:35

Document the compiler generated postblit types

19bdc68

RazvanN7 force-pushed the patch-9 branch from a5066d3 to 19bdc68 Compare March 22, 2018 13:03

RazvanN7 added 2 commits March 22, 2018 16:26

Add paragraph for non-overloadable this(this)

a272807

Add individual qualifier documentation

3e7965e

A few fixups

7c10ca4

andralex requested changes Mar 22, 2018

View reviewed changes

JinShil added the Vision https://wiki.dlang.org/Vision/2018H1 label Mar 23, 2018

RazvanN7 added 2 commits March 23, 2018 12:41

Apply review changes

86dee93

Add qualifier grouping table

0ab9071

RazvanN7 force-pushed the patch-9 branch from 8d79973 to 0ab9071 Compare March 23, 2018 13:27

andralex approved these changes Mar 23, 2018

View reviewed changes

andralex merged commit 36f7800 into dlang:master Mar 23, 2018

Uh oh!

Update struct.dd - postblit documentation #2281

Update struct.dd - postblit documentation #2281

Uh oh!

Conversation

RazvanN7 commented Mar 19, 2018

Uh oh!

dlang-bot commented Mar 19, 2018

Bugzilla references

Uh oh!

andralex commented Mar 19, 2018

Uh oh!

andralex commented Mar 19, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 22, 2018

Uh oh!

wilzbach commented Mar 22, 2018

Uh oh!

RazvanN7 commented Mar 22, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wilzbach commented Mar 30, 2018 •

edited

Loading