Skip to content

Fix issue 16970 - Fix deprecations and warnings when compiling Phobos#4956

Merged
dlang-bot merged 1 commit intodlang:masterfrom
edi33416:fix_deprecations
Dec 27, 2016
Merged

Fix issue 16970 - Fix deprecations and warnings when compiling Phobos#4956
dlang-bot merged 1 commit intodlang:masterfrom
edi33416:fix_deprecations

Conversation

@edi33416
Copy link
Contributor

Fixed ddocs in order to remove warnings when compiling html phobos, as mentioned on bugzilla.

I did not touch std/experimental/ndslice, since it's going to be deprecated.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
16970 Fix deprecations and warnings when compiling Phobos

* Consider using `getAddress`, `parseAddress` and `Address` methods
* instead of using this class directly.
*
* Example:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Example from ddoc and just made the unittest part of the docs, because:

  • it was doing the same thing
  • in case of a change in the API, we do not need to worry about modifying the example as well (that is why we have ///unittest)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes in general ddoced examples are horrible, because we can't ensure that they even compile.
As I seem not to be able to add a comment in the lines outside of the diff: maybe you could replace the commented writefln with assert's, so that it won't look to weird to a reader?
Note that for the rendered documentation we are considering to replace assert's like a == b automatically with a writeln equivalent

Copy link
Contributor

@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.

@@@BUG@@@ immediately caught my attention ;-)

Unlike $(LREF takeExactly), `take` does not require that there
are `n` or more elements in `r`. As a consequence, length
information is not applied to the result unless `r` also has
Unlike `takeExactly`, `take` does not require that there
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the motivation for removing LREF? It creates nice, clickable __L__ocal reference, e.g.:

http://dlang.org/phobos/std_range.html#.take

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, just a mistake due to rushing. Will undo

!is(R T == Take!T))
{
// @@@BUG@@@
//return input[0 .. min(n, $)];
Copy link
Contributor

Choose a reason for hiding this comment

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

You could try

static if (is(typeof(R.init[0 .. $]) == R))
    return input[0 .. min(n, $)];
else
   return input[0 .. min(n, input.length)];

(and obviously add a couple of tests for it)

Copy link
Contributor Author

@edi33416 edi33416 Dec 15, 2016

Choose a reason for hiding this comment

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

Could you explain what is the difference between the two return statements?
I thought that $ and length are equivalent.

Are you considering the fact that a range might define opDollar, but not length? If so, the hasSlicing template constraint would be false.

I am starting to think that this bug comment is a residue

Copy link
Member

Choose a reason for hiding this comment

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

Some ranges define length but not opDollar. IMHO we shouldn't cater to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that $ and length are equivalent.

Unfortunately they are not - opDollar needs to be defined separately :/
The rationale is that thus one can have special overloads that can be faster.
However, this makes using $ very, very error-prone - even in Phobos:

https://issues.dlang.org/show_bug.cgi?id=16073

IMHO we shouldn't cater to them.

Yes also it would be nice if opDollar would automatically alias to length if not defined and there is length defined.

I am starting to think that this bug comment is a residue

Well kind of. It's still true that opDollar simply might not be defined if hasSlicing is true, but there's no point of having this comment there
-> leave as it is currently and just remove the bug comment ;-)

* Consider using `getAddress`, `parseAddress` and `Address` methods
* instead of using this class directly.
*
* Example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes in general ddoced examples are horrible, because we can't ensure that they even compile.
As I seem not to be able to add a comment in the lines outside of the diff: maybe you could replace the commented writefln with assert's, so that it won't look to weird to a reader?
Note that for the rendered documentation we are considering to replace assert's like a == b automatically with a writeln equivalent

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

@wilzbach thx please approve as well when happy

!is(R T == Take!T))
{
// @@@BUG@@@
//return input[0 .. min(n, $)];
Copy link
Member

Choose a reason for hiding this comment

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

Some ranges define length but not opDollar. IMHO we shouldn't cater to them.

@wilzbach
Copy link
Contributor

thx please approve as well when happy

@edi33416 please remove the bug comment and squash - we should be good to go ;-)



///
@safe unittest
Copy link
Contributor

Choose a reason for hiding this comment

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

We now verify with CircleCi that public examples are runnable (also part of the style Makefile target)

out/std_socket.d(13): Deprecation: std.socket.softUnittest is not visible from module std_socket
out/std_socket.d(13): Error: function std.socket.softUnittest is not accessible from module std_socket

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 missed that softUnittest has a private scope. Fixed it

Copy link
Contributor

@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 a lot @edi33416

@dlang-bot dlang-bot merged commit 032d04d into dlang:master Dec 27, 2016
@edi33416 edi33416 deleted the fix_deprecations branch October 9, 2017 14:15
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.

4 participants