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

Improve destroy's documentation #2054

Merged
merged 1 commit into from
Jan 27, 2018
Merged

Improve destroy's documentation #2054

merged 1 commit into from
Jan 27, 2018

Conversation

JinShil
Copy link
Contributor

@JinShil JinShil commented Jan 23, 2018

The current documentation for destroy says that it sets the object to an "invalid state", but that's not what I've found in my testing, so I changed the documentation to explain what I think is happening, and added an example for illustration.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @JinShil!

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.

src/object.d Outdated
destroy(i);
assert(i == 0); // `i` is back to its initial state `0`
}
----
Copy link
Member

@wilzbach wilzbach Jan 25, 2018

Choose a reason for hiding this comment

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

Why don't you use a ddoc-ed unittest (or multiple)?

/// reference type demonstration
unittest
{

}

/// value type demonstration
unittest
{

}

They have the advantage of being

  • checked by the testsuite -> guaranteed the code will work in the future
  • runnable by the reader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed example to unittest blocks. I hope that's what you meant.

Copy link
Member

Choose a reason for hiding this comment

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

Nope it's not. You are still having it as a comment. I added a commit with what I meant.
Ddoc-ed unittest start with /// (or /++, /**) and will be part of the documentation, see e.g.

https://dlang.org/phobos/std_algorithm_iteration.html

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks.
FYI: I fixed the typo of "Refernce" in a force-pushed ammened commit.

does is done and so that it no longer references any other objects. It does
$(I not) initiate a GC cycle or free any GC memory.
+/
/********
Copy link
Member

Choose a reason for hiding this comment

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

We really need to define a best practice for docstrings.
Generally I'm a fan of using only as many * / + as strictly necessary:

/++

+/

or

/**

*/

Copy link
Member

Choose a reason for hiding this comment

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

Same here. Not a fan of lots of stars or stars at the beginning of each comment line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following @andralex's prior suggestion here: dlang/dmd#6835 (comment)

Let's use https://github.com/dlang/phobos/blob/master/std/string.d#L2254-L2267 for new code, thanks.

Anyway, if it's important, please make a PR to the style guide with preferred dos and don'ts

Copy link
Member

Choose a reason for hiding this comment

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

@wilzbach wilzbach merged commit bc75aa0 into dlang:master Jan 27, 2018
@JinShil JinShil deleted the destroy_doc branch April 24, 2018 10:28
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.

4 participants