Deprecate public aliases for Regex, StaticRegex, etc #5964

dhasenan · 2017-12-25T15:31:25Z

As per the discussion at #5963, we want the option to swap out the types for Regex, Capture, etc. This is impossible if people are able to refer to Regex by name. Furthermore, according to @wilzbach, aliases are not an option. Therefore, this PR deprecates public uses of regex types (Regex, StaticRegex, Captures, and RegexMatch) to ensure we can later swap out the implementation types..

@wilzbach

As per the discussion at dlang#5963, we want the option to swap out the types for Regex, Capture, etc. This is impossible if people are able to refer to Regex by name. Furthermore, according to @wilzbach, aliases are not an option. Therefore, this PR deprecates public uses of the Regex types to allow us to switch out the concrete type later.

dlang-bot · 2017-12-25T15:31:25Z

Thanks for your pull request, @dhasenan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

DmitryOlshansky · 2017-12-25T15:36:49Z

I think this quickly got out of hand. I think it's fine to keep Regex!Char and in fact I'd drop isRegexFor instead.

StaticRegex!X is indeed on its way out.

Captures!R is a non-standard concept (unlike e.g. forward range) that we are pretty much stuck with.

RegexMatch is indeed just a forward range of Captures so can deprecate that .

DmitryOlshansky · 2017-12-25T15:34:17Z

std/regex/package.d

 public alias StaticRegex = Regex;

+private alias _Regex(Char) = std.regex.internal.ir.Regex!(Char);


Btw that's another advantage of using auto:
We don't have to pay any initialization cost until the function is called once :)

You mean import cost, no? Because "initializers" would be constructors, usually. You never pay for an instance constructor until an instance is created, and you always pay for a static constructor before main().

Anyway, that's easily addressed:

template _Regex(Char) { private import std.regex.internal.ir : IRRegex = Regex; alias _Regex = IRRegex!Char; }

Pretty sure this technique has been discussed on the newsgroup several times.

I meant CTFE "initialization" cost - evaluation would have been a better word.

Pretty sure this technique has been discussed on the newsgroup several times.

Well NG and actual PRs is a huge difference. Here's an overview on the status quo on making Phobos slicker:

Faster to import std.regex, by lazy eval of wordMatcher #5942

Dramatically faster to compile Tuple #5931

Make import of std.internal.unicode_tables lazy. #5945

Dramatically faster to import std.datetime (and anything under it) #5948

Add a global convenience package file #5916

http://forum.dlang.org/post/zezhvfdwutpdqkbkjymp@forum.dlang.org (latest NG thread)

tl;dr: there are still a lot of easy fruits to pick :)

DmitryOlshansky · 2017-12-25T15:37:35Z

The biggest problem is that we should move docs to package.d so that Regex alias contains DDoc from internal/ir.d

dhasenan · 2017-12-25T16:05:43Z

I think this quickly got out of hand.

Thank you! I strive to be efficient.

I think it's fine to keep Regex!Char

But then we can't change regex() to return a value of a different type without a deprecation cycle. Since the consensus seems to be that this is an important thing for us to be able to do, we should get the necessary deprecation cycle started ASAP.

RegexMatch is indeed just a forward range of Captures so can deprecate that .

Not quite. It's a forward range of Captures with extra convenience methods pre, post, and hit. While we could add free functions to be used with UFCS to emulate this if we change to a more widely used type, this would break code that uses selective imports. Selective local import style is being pushed rather strongly.

So we can't switch from RegexMatch to something that's currently more widely used. But we could come up with a type similar to RegexMatch that can be more widely used (eg a more advanced type for searching through random-access ranges). If we did that, I would be able to add an alias for compatibility, but that apparently isn't a universal trait of phobos devs and I can't commit to being available to help out. So that's not an option.

wilzbach · 2017-12-25T16:59:05Z

I strive to be efficient.

https://issues.dlang.org/buglist.cgi?component=phobos&list_id=218415&product=D&resolution=---

Any help would be highly appreciated!

Furthermore, according to @wilzbach, aliases are not an option.
...
I would be able to add an alias for compatibility, but that apparently isn't a universal trait of phobos devs

Oh I think you have misunderstood me a bit. I was trying to say that we can't use aliases to change the return type of a function if it's not auto, e.g.:

Bar myFancyPhobosFunction()
{
...
}

and in a later version do the following:

alias Bar = Bar2;

auto essentially means to a developer "hey the return type of this function might change, use typeof(..) if you really need to store my type".

dhasenan · 2017-12-25T17:26:30Z

I strive to be efficient.

https://issues.dlang.org/buglist.cgi?component=phobos&list_id=218415&product=D&resolution=---

Any help would be highly appreciated!

Ah, I see the problem. When I said I strive to be efficient, you thought I said that I strive to do whatever you think is important whenever you want, without compensation. I really have no clue how people keep confusing the two, but since they do it a lot, apparently it's an easy mistake to make.

I charge $150 per hour normally, but I offer a 30% open source discount.

I was trying to say that we can't use aliases to change the return type of a function if it's not auto

Some discussion of why this is the case would be salient. Removing one type in favor of another and adding a compatibility alias seems like the obvious way to make a change with a reasonable deprecation cycle.

Since it's apparently better to document the entire interface of your return types than to use static typing to direct people to the type's own documentation, this ensures that that documentation is complete.

schveiguy · 2017-12-31T14:43:31Z

FYI, here is a PR that fixes just the immediate issue for RegexMatch.front: #5976

auto essentially means to a developer "hey the return type of this function might change, use typeof(..) if you really need to store my type".

This is not correct IMO. Only Voldemort returns make this guaranteed. And even there, it doesn't give you liberty to change the API of the return type.

For instance, if you have:

auto foo(T)(T t)
{
   static if(is(T == int)) return cast(long)t * 2;
   else return t * 2;
}

auto doesn't say we can change the type without notice. It says "I don't want to have to make a template IntOrLong just to write a return type for this"

DmitryOlshansky

If we are to go forward with this:

tune down the destruction
remove 3 documented comments
move whatever documentation there is from internal subpackage

Lastly - for contructual work, negotiate with D Language foundation if they can accomodate such.

DmitryOlshansky · 2018-01-15T07:53:17Z

std/regex/package.d

@@ -557,57 +562,49 @@ public:
            }
        }
    }
-    ///Slice of input prior to the match.
+


This is an important part of API actually.

DmitryOlshansky · 2018-01-15T07:53:32Z

std/regex/package.d

    @property R pre()
    {
        return _nMatch == 0 ? _input[] : _input[0 .. matches[0].begin];
    }

-    ///Slice of input immediately after the match.
    @property R post()


DmitryOlshansky · 2018-01-15T07:54:32Z

std/regex/package.d

-        assert(!matchFirst("nothing", "something"));
-        ---
-    +/
-


Super useful part of API

DmitryOlshansky · 2018-01-15T07:54:53Z

std/regex/package.d

-        Number of pattern matched counting, where 1 - the first pattern.
-        Returns 0 on no match.
-    +/
-
    @safe @property int whichPattern() const nothrow { return _nMatch; }


Remarkably important part of API that allows you to match many things at once

DmitryOlshansky · 2018-01-15T07:55:16Z

std/regex/package.d

    @safe @property int whichPattern() const nothrow { return _nMatch; }

-    ///
    @system unittest


this is plain sabotage - deleting code that shows how to use the above powerful primitive

DmitryOlshansky · 2018-01-15T07:58:44Z

std/regex/package.d

    @property bool empty() const { return _captures._nMatch == 0; }

-    ///Same as !(x.empty), provided for its convenience  in conditional statements.


Useful short-cut to be documented.

DmitryOlshansky · 2018-01-15T08:02:15Z

std/regex/package.d

-    Effectively it's a forward range of Captures!R, produced
-    by lazily searching for matches in a given input.
-+/
-@trusted public struct RegexMatch(R)


Basically of this PR,
only this can be kept

DmitryOlshansky · 2018-01-15T08:02:41Z

std/regex/package.d

    @property R post()
    {
        return _captures.post;
    }

-    ///ditto
    @property R hit()


The 2-nd thing to keep from this PR

DmitryOlshansky · 2018-01-15T08:03:55Z

std/regex/package.d

+      $(LI captures: compatibility alias for previous versions of std.regex.)
+    )
+
+    Each capture struct offers the following methods:


Instead just document the Captures struct. This description is at best a skeleton

DmitryOlshansky · 2018-01-15T08:04:25Z

std/regex/package.d

    T opCast(T:bool)(){ return !empty; }

-    /// Same as .front, provided for compatibility with original std.regex.


The 3-rd thing to keep from this PR

dhasenan · 2018-01-15T16:34:44Z

Instead just document the Captures struct.

We can't use real return types because we need to be able to change out the types returned without a deprecation cycle. But in other places where that's done, we either use a private type or a Voldemort type to make it actually work. Nobody can actually refer to MapResult, so we can switch it out trivially.

That's not the case with Captures, so we need to make it private as soon as possible, since that will require a deprecation cycle and we don't want to have to wait in order to change the return types here.

However, once Captures is private, it won't appear in ddoc. Therefore we need to document it elsewhere.

(Or we could just be honest about our inclination to change return types and stop using auto, but that idea was shot down.)

Anyway, I don't have the patience to deal with open source anymore, so you all can decide what you want. I'm out.

dhasenan requested a review from DmitryOlshansky as a code owner December 25, 2017 15:31

DmitryOlshansky reviewed Dec 25, 2017

View reviewed changes

dlang-bot added the Needs Work label Dec 29, 2017

Move documentation to appropriate places.

0039ca2

Since it's apparently better to document the entire interface of your return types than to use static typing to direct people to the type's own documentation, this ensures that that documentation is complete.

dlang-bot added Needs Rebase and removed Needs Rebase labels Jan 1, 2018

DmitryOlshansky requested changes Jan 15, 2018

View reviewed changes

dhasenan closed this Jan 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate public aliases for Regex, StaticRegex, etc #5964

Deprecate public aliases for Regex, StaticRegex, etc #5964

dhasenan commented Dec 25, 2017

dlang-bot commented Dec 25, 2017

DmitryOlshansky commented Dec 25, 2017

DmitryOlshansky Dec 25, 2017

wilzbach Dec 25, 2017

dhasenan Dec 25, 2017 •

edited

wilzbach Dec 25, 2017

DmitryOlshansky commented Dec 25, 2017

dhasenan commented Dec 25, 2017 •

edited

wilzbach commented Dec 25, 2017

dhasenan commented Dec 25, 2017 •

edited

schveiguy commented Dec 31, 2017

DmitryOlshansky left a comment

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

DmitryOlshansky Jan 15, 2018

dhasenan commented Jan 15, 2018

		public alias StaticRegex = Regex;

		private alias _Regex(Char) = std.regex.internal.ir.Regex!(Char);

		@property bool empty() const { return _captures._nMatch == 0; }

		///Same as !(x.empty), provided for its convenience in conditional statements.

		T opCast(T:bool)(){ return !empty; }

		/// Same as .front, provided for compatibility with original std.regex.

Deprecate public aliases for Regex, StaticRegex, etc #5964

Deprecate public aliases for Regex, StaticRegex, etc #5964

Conversation

dhasenan commented Dec 25, 2017

dlang-bot commented Dec 25, 2017

Bugzilla references

DmitryOlshansky commented Dec 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhasenan Dec 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryOlshansky commented Dec 25, 2017

dhasenan commented Dec 25, 2017 • edited

wilzbach commented Dec 25, 2017

dhasenan commented Dec 25, 2017 • edited

schveiguy commented Dec 31, 2017

DmitryOlshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhasenan commented Jan 15, 2018

dhasenan Dec 25, 2017 •

edited

dhasenan commented Dec 25, 2017 •

edited

dhasenan commented Dec 25, 2017 •

edited