Skip to content

Comments

dtemplate Tuple, add a ctor taking num elements#8554

Merged
dlang-bot merged 2 commits intomasterfrom
unknown repository
Aug 13, 2018
Merged

dtemplate Tuple, add a ctor taking num elements#8554
dlang-bot merged 2 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Aug 10, 2018

No description provided.

@dlang-bot
Copy link
Contributor

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

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#8554"


extern (D) this(){}

extern (D) this(size_t numObjects)
Copy link
Contributor

@JinShil JinShil Aug 10, 2018

Choose a reason for hiding this comment

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

We're trying to incrementally improve the dmd code base. One of the things we prefer is that any new public symbols have full DDoc headers with a description, params, and returns. I admit it's selectively enforced, and I don't think it should prevent a PR from being merged, but we would like contributors to put forth the effort. 😉

EDIT: Also please see https://dlang.org/dstyle.html#phobos_documentation. It's different from the rest of DMD but it's the style we want to incrementally migrate to (at least some of us).

Copy link
Author

Choose a reason for hiding this comment

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

Yes of course. This is just a bit strange to start doing it when the practice is not generalized.
I had to amend anyway since the default ctor can be avoided using a default numObjects.

* Params:
* numObjects = The initial number of objects.
*/
extern (D) this(size_t numObjects = 0)
Copy link
Contributor

@JinShil JinShil Aug 10, 2018

Choose a reason for hiding this comment

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

See item 12 here: https://forum.dlang.org/post/p6oibo$1lmi$1@digitalmars.com

I don't really like it, but you can avoid the default parameter by forwarding to a parameter-less constructor in the body of this constructor.

Edit: Of course, he also says "Minimize use of overloading." and "treating the above as sacred writ is a huge mistake.", so maybe we should keep it a default parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Yes but it's either that or a useless default ctor since default ctors ares till not auto generated ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing the preference would be:

/**
Constructs a new instance.
*/
extern (D) this()
{
    this(0);
}

/**
Constructs a new instance.

Params:
    numObjects = The initial number of objects.
*/
extern (D) this(size_t numObjects)
{
    objects.setDim(numObjects);
}

Copy link
Author

Choose a reason for hiding this comment

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

No, with empty parens this would mean two useless call: other ctor overload and then objects.setDim.

Objects objects;

/**
* Constructs a new instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the prepended *.


/**
* Constructs a new instance.
* Params:
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline before params

Copy link
Contributor

Choose a reason for hiding this comment

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

We really should have an updated style guide for all this that is linked from the readme or the contribute documentation.

extern (D) this() {}

/**
* Constructs a new instance.
Copy link
Member

Choose a reason for hiding this comment

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

This line is spurious, as that's what a this constructor does. Please delete it.

Copy link
Author

Choose a reason for hiding this comment

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

ok done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all documented symbols should have a summary. This allows to automatically generate an index table, i.e. something like the table at the top of [1].

[1] https://dlang.org/phobos/std_array.html

@Geod24 Geod24 closed this Aug 13, 2018
@Geod24 Geod24 reopened this Aug 13, 2018
@dlang-bot dlang-bot merged commit ecde30e into dlang:master Aug 13, 2018
@ghost ghost deleted the tup-ctor branch August 13, 2018 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants