Skip to content

Conversation

@JohanEngelen
Copy link
Contributor

This fixes a warning one gets when compiling with warnings enabled:
"phobos/std/algorithm/sorting.d(3122): Warning: calling testcase.foo.__lambda1 without side effects discards return value of type bool, prepend a cast(void) if intentional"

Testcase that fails compilation with -w:

import std.algorithm.sorting;

auto foo() {
    int[] v = [ 25, 7, 9, 2, 0, 5, 21 ];
    return topN!((a, b) => a > b)(v, 100);
}

Phobos unittests are compiled with -w right? In that case, the expanded testcase takes care of testing this fix.

This fixes a warning one gets when compiling with warnings enabled:
"phobos/std/algorithm/sorting.d(3122): Warning: calling testcase.foo.__lambda1 without side effects discards return value of type bool, prepend a cast(void) if intentional"

Testcase that fails compilation with `-w`:
```
import std.algorithm.sorting;

auto foo() {
    int[] v = [ 25, 7, 9, 2, 0, 5, 21 ];
    return topN!((a, b) => a > b)(v, 100);
}
```
Passing the comparison operation as string "a < b" is tested a few lines up. This adds a test for passing a lambda function.
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @JohanEngelen! 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.

@wilzbach
Copy link
Contributor

wilzbach commented Dec 15, 2017

Phobos unittests are compiled with -w right? In that case, the expanded testcase takes care of testing this fix.

Yes. FYI you can easily check this yourself:

>  make -f posix.mak std/algorithm/sorting.test
...
../dmd/generated/linux/release/64/dmd -conf= -I../druntime/import  -w -de -dip25 -m64 ...
...
std/algorithm/sorting.d(3122): Warning: calling std.algorithm.sorting.__unittest_std_algorithm_sorting_d_3134_45.__lambda1 without side effects discards return value of type bool, prepend a cast(void) if intentional
make: *** [posix.mak:385: std/algorithm/sorting.test] Error 1

// Safety checks: enumerate all potentially unsafe generic primitives
// then use a @trusted implementation.
binaryFun!less(r[0], r[r.length - 1]);
cast(void) binaryFun!less(r[0], r[r.length - 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW this looks like unfinished work. This is in an if (false) block ...

@JohanEngelen
Copy link
Contributor Author

Can one of you retrigger the failed testers so this is merged? Thanks.

@PetarKirov
Copy link
Member

Sure, done. Ping again if you need help.

@JohanEngelen
Copy link
Contributor Author

JohanEngelen commented Dec 16, 2017

Thanks.

What's wrong on CircleCI: Dscanner crashes on this code??

@PetarKirov
Copy link
Member

Dscanner has random chance of crashing on any PR. We're running it under DScanner in order to try to narrow down the problem, as it doesn't reproduce locally...

@wilzbach
Copy link
Contributor

Sadly you aren't that special, it's DScanner has been segfaulting spuriously for quite a while.

See e.g. #5929

As I have actually restarted your PR more then three times yesterday, let's try to temporarily ignore the failing DScanner (-> #5933), s.t. we aren't blocked here.

@JohanEngelen
Copy link
Contributor Author

Thanks.
FYI, this issue may seem trivial, but actually it is very bad in practice: it prevents upgrading the compiler if one is building with -w, warnings==errors; and there is no viable workaround other than to not use that particular Phobos function with a lambda.
(it is similar for the deprecation messages that I see in the build log when building phobos/druntime !)

@wilzbach
Copy link
Contributor

(it is similar for the deprecation messages that I see in the build log when building phobos/druntime !)

Hehe, I'm aware and you shouldn't see any for Phobos since this summer: #5546 (we now build with -de by default).

There's a similar PR for druntime, but I haven't figured out yet how to fix the deprecations for Martin's new arrayop package.

@wilzbach
Copy link
Contributor

Force-merging this due to our CircleCi/DScanner SEGFAULT issue. We should probably go with #5546 for now :/

@wilzbach wilzbach merged commit 81c50e3 into dlang:master Dec 17, 2017
@JohanEngelen JohanEngelen deleted the patch-1 branch December 17, 2017 11:43
JohanEngelen added a commit to weka/phobos that referenced this pull request Dec 28, 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.

5 participants