Skip to content

Conversation

@wilzbach
Copy link
Contributor

I know that this feels wrong (it is!), but we frequently get this error on auto-tester:

make[1]: *** [generated/linux/release/64/unittest/std/regex/internal/tests.o] Killed
  1. Brad can't / doesn't want to increase the RAM (it's only 2GB)

See also: braddr/d-tester#66, braddr/d-tester#69

  1. newCTFE still seems far away from becoming reality (CC @UplinkCoder)

  2. We already tried splitting it up into two pieces, but tests.d seems to be the big part (Split-up regex tests to please auto-tester #6061)

  3. At the moment only a part of the testsuite is actually run at CT:

enum Tests = chain(iota(0, 30), iota(235, tv.length-5));
static foreach (v; Tests)
  1. In theory, we could just use nice version flags to inject the values. That's already tricky in GNUMake and even trickier in DigitalMars Make.

=> I guess we are left to splitting it up again. As currently it already fails with only 35 elements of the queue, I went for 10 elements per file.


Disclaimer: I don't want to do this, but we gotta fight the CI errors somehow.


Better ideas are more than welcome!

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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.

@wilzbach wilzbach force-pushed the splitup-regex branch 3 times, most recently from f1ee990 to f85f2ef Compare February 12, 2018 23:20
@DmitryOlshansky
Copy link
Member

Better ideas are more than welcome!

Another idea is to deprecate ctRegex and stop testing it. It's 100% obvious that it doesn't work on anything significant in practice due to our compiler limitations. Then we enable it again once CTFE works with std.regex.

ATM it's not even remotely fully tested in CI and I feel guilty for advertising things we do not test.
Add to that std.regex itself is not 100% covered with the FULL test suite. What kind of guarantees we have about ctRegex then?

@wilzbach
Copy link
Contributor Author

Another idea is to deprecate ctRegex and stop testing it. It's 100% obvious that it doesn't work on anything significant in practice due to our compiler limitations. Then we enable it again once CTFE works with std.regex.

Sounds very reasonably to me, though I'm not sure on whether we should actually trigger the deprecation message or just remove it from the docs.

CC @andralex @CyberShadow

@CyberShadow
Copy link
Member

Undocumenting it, perhaps, but removing it might be too much. Move it to its separate module, maybe? The compiler is very likely to only keep improving.

As a semi-aside, and I might be completely wrong about this, but one thing I've noticed to be receiving a lot of attention is Unicode-correctness in std.regex. Although that is a great goal, I'm not sure if the price we're paying for it is making it worthwhile. How much simpler and faster would std.regex be if it only cared about ASCII? How many use cases involving regular expressions actually need to be Unicode-aware?

@JackStouffer
Copy link
Contributor

The rest of the language is unicode correct (mostly). I don't think we can justify not supporting it when D is built around it, and our higher level competitors support it as well

@wilzbach
Copy link
Contributor Author

wilzbach commented Jun 7, 2018

Closing this as it's not the real fix, but we really should solve the problem of ctRegex or at least deprecate it for now.

@wilzbach wilzbach closed this Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants