Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve documentation of SortedRange.release #8066

Merged
merged 1 commit into from
May 21, 2021

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented May 11, 2021

Because it came up on the forum that the documentation of release is not informative: When should I use SortedRange.release?

@dlang-bot
Copy link
Contributor

dlang-bot commented May 11, 2021

Thanks for your pull request and interest in making D better, @dkorpel! 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 coverage diff by visiting the details link of the codecov check)
  • 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 run digger -- build "master + phobos#8066"

Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

A rebase will most likely get rid of the tester failures.

///
@safe unittest
{
auto a = assumeSorted([ 1, 2, 3 ]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that adding:

assert(a == sort!"a < b"([1, 2, 3]));

Might clarify that a is a sorted range, whereas release simply extracts the underlying array.

@dkorpel dkorpel force-pushed the sorted-range-release branch 4 times, most recently from 0c872f9 to 5d5df69 Compare May 12, 2021 13:29
@RazvanN7
Copy link
Collaborator

hm... both circleCI and buildkite failt phobos with:

std/typecons.d(1246): Error: `TypeInfo` cannot be used with -betterC

This obviously has nothing to do with this PR.

@RazvanN7
Copy link
Collaborator

Hmm, it looks like the error might not be spurious after all. Could you try commenting out the unittest to see if the autotester still fails?

@RazvanN7
Copy link
Collaborator

@dkorpel What happened with the testing pipeline when the unittest was commented?

@dkorpel
Copy link
Contributor Author

dkorpel commented May 19, 2021

It was all green with the test commented out

@dkorpel
Copy link
Contributor Author

dkorpel commented May 19, 2021

And now it's red again. I don't understand what's buildkite's problem with Phobos, the log ends in:

0.000s PASS release64 std.internal.scopebuffer
0.001s PASS release64 std.internal.test.dummyrange
0.000s PASS release64 std.internal.test.range
0.000s PASS release64 std.typetuple
make[1]: Leaving directory '/var/lib/buildkite-agent/builds/ci-agent-3ff56b73-5108-4d1c-a54b-acc66981efad-8/dlang/phobos/phobos'
rm generated/linux/release/64/betterctests/.directory
🚨 Error: The command exited with status 2

So it fails to remove the betterC tests?

@RazvanN7
Copy link
Collaborator

RazvanN7 commented May 20, 2021

It's the same as circleCI. My assumption is that the test should be @BetterC

../dmd/generated/linux/release/64/dmd  -conf= -I../druntime/import  -w -de -preview=dip1000 -preview=dtorfields -m64 -fPIC -O -release -defaultlib= -debuglib= -L-lpthread -L-lm -betterC -unittest -run generated/linux/release/64/betterctests/std_range_package.d
--
&nbsp; | std/typecons.d(1246): Error: `TypeInfo` cannot be used with -betterC
&nbsp; | posix.mak:668: recipe for target 'std/range/package.betterc' failed

@MoonlightSentinel
Copy link
Contributor

It's the same as circleCI. My assumption is that the test should be @BetterC

Yes, the unittest is nested in SortedRange and hence pulled into the @BetterC test declared here.

Use a static array instead of a dynamically allocated one and it should pass the test.

@dkorpel
Copy link
Contributor Author

dkorpel commented May 20, 2021

Yes, the unittest is nested in SortedRange and hence pulled into the @BetterC test declared here.

Ugh, this is why I never use nested unittests. I wish there was a way to make a top-level unittest work like a DDoc code example for a member function.

@dkorpel
Copy link
Contributor Author

dkorpel commented May 20, 2021

It's still failing. Is there a way to see the circleci build log without giving them complete read/write access to all my private repositories?

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented May 20, 2021

std/typecons.d(1246): Error: `TypeInfo` cannot be used with -betterC
posix.mak:668: recipe for target 'std/range/package.betterc' failed

You can run these tests locally,

Run all betterC tests

make -f posix.mak betterc

Only the failing test:

make -f posix.mak std/range/package.betterc

@RazvanN7
Copy link
Collaborator

generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4file8DirEntry6isFileMFNdNfZb'
--
&nbsp; | generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4file8DirEntry4nameMxFNaNbNdNfZAya'
&nbsp; | generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4file8DirEntry4nameMxFNaNbNdNfZAya'
&nbsp; | generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4file8DirEntry4nameMxFNaNbNdNfZAya'
&nbsp; | generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4file8DirEntry4nameMxFNaNbNdNfZAya'
&nbsp; | generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4path12absolutePathFNaNfAyaLQeZQh'
&nbsp; | generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench8runTestsFSQu6ConfigZv: error: undefined reference to '_D3std4path12absolutePathFNaNfAyaLQeZQh'

I'm fairly certain that after this last rebase, this will be good to go.

@dkorpel
Copy link
Contributor Author

dkorpel commented May 21, 2021

CircleCI and buildkite are happy now, so it's Azure's turn to error out:

std.utf.UTFException@D:\a\1\dmd\ldc2-1.23.0-windows-multilib\bin\..\import\std\utf.d(1507): Invalid UTF-8 sequence (at index 1)
----------------
0x002B7E0B in ulong[] core.sys.windows.stacktrace.StackTrace.traceNoSync(uint, core.sys.windows.winnt.CONTEXT*)
0x002B7BC2 in core.sys.windows.stacktrace.StackTrace core.sys.windows.stacktrace.StackTrace.__ctor(uint, core.sys.windows.winnt.CONTEXT*)
0x002ABA9D in object.Throwable.TraceInfo core.runtime.defaultTraceHandler(void*)

@dlang-bot dlang-bot merged commit 460ed9c into dlang:master May 21, 2021
@dkorpel dkorpel deleted the sorted-range-release branch May 21, 2021 14:28
@RazvanN7
Copy link
Collaborator

That was an unexpected ride for a documentation PR. Thanks for your patience @dkorpel !

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