Skip to content

Comments

Restrict length#4827

Merged
wilzbach merged 1 commit intodlang:masterfrom
andralex:length
Jun 16, 2017
Merged

Restrict length#4827
wilzbach merged 1 commit intodlang:masterfrom
andralex:length

Conversation

@andralex
Copy link
Member

@andralex andralex commented Oct 1, 2016

Fixes https://issues.dlang.org/show_bug.cgi?id=16566. hasLength was in fact intended to allow only integrals, but its code predates alias this and we didn't update it.

Throughout Phobos we use interchangeably auto s = r.length;, size_t s = r.length;, cache the length of a range in a variable, or just use it as a size_t. Supporting other types than size_t meaningfully would put too much burden on the definition and the implementation.

@andralex andralex force-pushed the length branch 2 times, most recently from 6fb2481 to aa34160 Compare October 1, 2016 02:36
@jmdavis
Copy link
Member

jmdavis commented Oct 5, 2016

Shouldn't the restriction be that it be size_t only and not ulong? We even removed the ability for iota to have a length of ulong (unless size_t is ulong) because of the problems that it caused.

@burner
Copy link
Member

burner commented Oct 11, 2016

the doc unittest could use some more failing asserts

@JackStouffer
Copy link
Contributor

Ping @andralex

@9il
Copy link
Member

9il commented Nov 12, 2016

ping @andralex

@andralex
Copy link
Member Author

I plan to be more aggressive about this: if an artifact defines length to have type ulong on a 32-bit system, I'll yield a deprecation message. @MartinNowak does this work for you?

@JackStouffer
Copy link
Contributor

Ping @MartinNowak

This is because a narrow string's length does not reflect the number of
characters, but instead the number of encoding units, and as such is not useful
with range-oriented algorithms. To use strings as random-access ranges with
length, use $(XREF string, representation) or $(XREF utf, byCodeUnit). */
Copy link
Contributor

Choose a reason for hiding this comment

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

The XREF macro was deprecated, please use the universal REF, e.g. `$(REF, byCodeUnit, std, utf)

@wilzbach
Copy link
Contributor

wilzbach commented Feb 17, 2017

I plan to be more aggressive about this: if an artifact defines length to have type ulong on a 32-bit system, I'll yield a deprecation message.

How about hacking template constraints together for a pseudo-deprecation message as we deprecated on templates doesn't seem to work:

private enum bool oldHasLengthImpl(R) = !isNarrowString!R && is(typeof(
      (inout int = 0)
      {
          R r = R.init;
          ulong l = r.length;
      }));

private enum bool newHasLengthImpl(R) = !isNarrowString!R && is(typeof(
      (inout int = 0)
      {
          R r = R.init;
          auto x = r.length;
          static assert(is(typeof(x) == size_t) || is(typeof(x) == ulong));
      }));

template hasLength(R)
if (oldHasLengthImpl!R && newHasLengthImpl!R)
{
    enum bool hasLength = true;
}

template hasLength(R)
if (oldHasLengthImpl!R && !newHasLengthImpl!R)
{
    pragma(msg, "Deprecation: length of " ~ R.stringof ~ " needs to return size_t or ulong");
    enum bool hasLength = true;
}

template hasLength(R)
if (!oldHasLengthImpl!R)
{
    enum bool hasLength = false;
}

In Phobos there is only this one violation in std.uni.simpleCaseFoldings

@wilzbach wilzbach added this to the 2.075.0 milestone Mar 4, 2017
@quickfur
Copy link
Member

ping @andralex

@andralex andralex force-pushed the length branch 3 times, most recently from cf8223e to 7d411b0 Compare June 6, 2017 21:41
@andralex andralex force-pushed the length branch 2 times, most recently from da69804 to 0e86125 Compare June 7, 2017 01:15
@andralex
Copy link
Member Author

andralex commented Jun 7, 2017

OK, this is reviewable now. The new solution is nicer than the inout trick and uses a deprecation step.

{
// Uncomment the deprecated(...) message and take the pragma(msg)
// out once https://issues.dlang.org/show_bug.cgi?id=10181 is fixed.
pragma(msg, __FILE__ ~ "(" ~ __LINE__.stringof ~
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably include an @@@deprecated... comment and deprecation end date here, so that one can easily grep for it and remove this bit eventually.

// Uncomment the deprecated(...) message and take the pragma(msg)
// out once https://issues.dlang.org/show_bug.cgi?id=10181 is fixed.
pragma(msg, __FILE__ ~ "(" ~ __LINE__.stringof ~
"): Deprecation: length must have type size_t on all systems");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not imitate the compiler's message format, as it might be confusing for people compiling with deprecation warnings as errors, or who have them disabled entirely. LGTM otherwise.

@andralex
Copy link
Member Author

andralex commented Jun 7, 2017

Updated

{
enum bool hasLength = !isNarrowString!R;
}
else static if (is(Length : ulong))
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you mix the Length == size_t and Length : ulong styles?

Copy link
Member Author

Choose a reason for hiding this comment

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

These aren't styles. == checks for exact match, : is convertibility.

range-oriented algorithms.
*/
Yields `true` if `R` has a `length` member that returns a value of either
`size_t` or `ulong` type. `R` does not have to be a range. If `R` is a range,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be updated as well if you deprecate ulong?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ok will update text

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
16566 hasLength should enforce that length has type size_t

@wilzbach
Copy link
Contributor

@andralex I changed your commit message, s.t. the bug can detect the issue and added a small changelog entry (basically just copied your last paragraph).

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.

Let's get this shipped ;-)

@wilzbach wilzbach merged commit 625dfee into dlang:master Jun 16, 2017
// @@@DEPRECATED_2017-12@@@
// Uncomment the deprecated(...) message and take the pragma(msg)
// out once https://issues.dlang.org/show_bug.cgi?id=10181 is fixed.
pragma(msg, __FILE__ ~ "(" ~ __LINE__.stringof ~
Copy link
Member

Choose a reason for hiding this comment

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

How is the FILE and LINE of std.range.primitives interesting here? You'd need to pass them as defaults for template arguments to get the instantiating side.

Copy link
Member Author

Choose a reason for hiding this comment

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

They emulate what the deprecated facility does.

MartinNowak added a commit to andralex/phobos that referenced this pull request Jun 17, 2017
andralex pushed a commit to andralex/phobos that referenced this pull request Jul 13, 2017
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.

10 participants