Skip to content

Comments

std.range.package.d: -dip1000 compilable#6396

Closed
carblue wants to merge 1 commit intodlang:masterfrom
carblue:std_range_package
Closed

std.range.package.d: -dip1000 compilable#6396
carblue wants to merge 1 commit intodlang:masterfrom
carblue:std_range_package

Conversation

@carblue
Copy link
Contributor

@carblue carblue commented Mar 31, 2018

https://github.com/dlang/phobos/blob/master/dip1000.mak with
aa[std.range.package]=-dip1000
Errors when running: make -f posix.mak std/range/package.test
...

T=`mktemp -d /tmp/.dmd-run-test.XXXXXX` &&                                                              \
  (                                                                                                     \
    ../dmd/generated/linux/release/64/dmd -od$T -conf= -I../druntime/import  -w -de -dip25 -m64 -fPIC -transition=complex -O -release -dip1000  -main -unittest generated/linux/release/64/libphobos2.a -defaultlib= -debuglib= -L-ldl -cov -run std/range/package.d ;     \
    RET=$? ; rm -rf $T ; exit $RET                                                                   \
  )
std/range/package.d(1703): Error: reference to local variable a assigned to non-scope parameter _param_1 calling std.range.chooseAmong!(RefAccessRange, RefAccessRange).chooseAmong
std/range/package.d(1703): Error: reference to local variable b assigned to non-scope parameter _param_2 calling std.range.chooseAmong!(RefAccessRange, RefAccessRange).chooseAmong
std/range/package.d(5089): Error: scope variable r1 assigned to non-scope parameter _param_0 calling std.range.zip!(NonSliceableRandomAccess, NonSliceableRandomAccess).zip
std/range/package.d(5089): Error: scope variable r2 assigned to non-scope parameter _param_1 calling std.range.zip!(NonSliceableRandomAccess, NonSliceableRandomAccess).zip
std/range/package.d(5090):        while evaluating: static assert(isRandomAccessRange!(typeof(__error)))

Maybe, the changes to the affected unittests are too drastic in that they aren't @nogc and don't operate with static arrays any more.
The PR shows, what's working without changing std.range algorithms.


Starting a countdown: The last 5 (6?) phobos modules, that haven't -dip1000 set (apart from those in the PR pipeline):
std.format
std.outbuffer
std.path
(std.algorithm.searching #6359 states, it depends on std.container.slist only)
std.algorithm.sorting see also #6359
std.experimental.logger.filelogger

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @carblue! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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 fetch digger
dub run digger -- build "master + phobos#6396"

int[2] b = [6, 5];
int[] a = [4, 3, 2, 1];
int[] b = [6, 5];
auto c = chooseAmong(0, RefAccessRange(a[]), RefAccessRange(b[]));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. [] is done here to create a slice of the static array (it's not needed for dynamic arrays, they are already slices)
  2. It feels like things like this here should be possible with DIP1000 :/

However, I looked at this problem myself two days ago and I couldn't find a better solution either.
So I think version(DIP1000) {} else at the top of the unittest might be better, because then we know that this still needs a look at, but aren't blocked by it.
What do you or others think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I tried both, a and a[] etc. and they both work; don't know why I added superfluous [] again, probably to be minimally invasive, expecting a change back to static arrays

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in order not to lose the @nogc and static array testing, "version(DIP1000) {} else" will be better, but I'll wait for a go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe https://github.com/D-Programming-Deimos/openssl needs an update to openssl 1.0.2g? That's what I have on my Kubuntu system since the day before yesterday

Copy link
Contributor

Choose a reason for hiding this comment

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

How is this related? (and most likely - I think the Deimos packages aren't super-actively maintained)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It relates to my reasoning why continuous-integration/jenkins/pr-merge might have failed:
...
Fetching openssl 1.1.6+1.0.1g (getting selected version)...
Package diet-ng can be upgraded from 1.4.3 to 1.4.5.
Package taggedalgebraic can be upgraded from 0.10.8 to 0.10.11.
Package stdx-allocator can be upgraded from 2.77.0 to 2.77.1.
Package vibe-d can be upgraded from 0.8.3-alpha.3 to 0.8.3.
Package memutils can be upgraded from 0.4.9 to 0.4.10.
Package vibe-core can be upgraded from 1.4.0-alpha.1 to 1.4.0.
Package eventcore can be upgraded from 0.8.27 to 0.8.34.
Use "dub upgrade" to perform those changes.
Non-optional dependency openssl of botan not found in dependency tree!?.
script returned exit code 2

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's an old Dub bug, see e.g.

dlang/dub#1180
dlang/dub#1112
dlang/dub#986

Unfortunately Jenkins with all the projects it tests is quite "vulnerable" to random failures.
OTOH it has caught tons of regressions, so nothing is for free :/

Anyhow I restarted the job.
BTW feel free to ping me on Slack or the people in #phobos when you run into this again.

@thewilsonator
Copy link
Contributor

#6765

I'm closing this one cos' its logged ~840 hours on the autotester in a single shot.

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.

4 participants