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

Qualify some function parameters under std.conv as const to reduce te… #8251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nordlow
Copy link
Contributor

@nordlow nordlow commented Sep 23, 2021

…mplate bloat

Trivial review, @thewilsonator.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @nordlow! 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#8251"

@PetarKirov
Copy link
Member

@nordlow unit-threaded fails to compile with this change:

Cloning https://github.com/atilaneves/unit-threaded (tag: v2.0.3)
Running the testsuite
~> case "$REPO_FULL_NAME" in
~> dub test
Running custom 'unittest' configuration.
Performing "unittest" build using /var/lib/buildkite-agent/builds/ci-agent-7d11ce47-aa1b-4308-a8a5-39b5bb0de700-2/dlang/phobos/build/distribution/bin/dmd for x86_64.
unit-threaded:from 2.0.3: building configuration "library"...
unit-threaded:exception 2.0.3: building configuration "library"...
unit-threaded:assertions 2.0.3: building configuration "library"...
unit-threaded:integration 2.0.3: building configuration "library"...
unit-threaded:mocks 2.0.3: building configuration "library"...
unit-threaded:property 2.0.3: building configuration "default"...
unit-threaded:runner 2.0.3: building configuration "library"...
subpackages/runner/source/unit_threaded/runner/factory.d(55,24): Error: template `std.conv.text` cannot deduce function from argument types `!()(string, const(TestData))`
/var/lib/buildkite-agent/builds/ci-agent-7d11ce47-aa1b-4308-a8a5-39b5bb0de700-2/dlang/phobos/build/distribution/bin/../imports/std/conv.d(4738,8):        Candidate is: `text(T...)(const(T) args)`
subpackages/runner/source/unit_threaded/runner/testcase.d(274,39): Error: template `std.conv.text` cannot deduce function from argument types `!()(string, string, string, const(TypeInfo), string, TypeInfo_Class)`
/var/lib/buildkite-agent/builds/ci-agent-7d11ce47-aa1b-4308-a8a5-39b5bb0de700-2/dlang/phobos/build/distribution/bin/../imports/std/conv.d(4738,8):        Candidate is: `text(T...)(const(T) args)`

@nordlow
Copy link
Contributor Author

nordlow commented Sep 24, 2021

subpackages/runner/source/unit_threaded/runner/factory.d(55,24): Error: template std.conv.text cannot deduce function from argument types !()(string, const(TestData))

I can't make sense of why this fails.

This passes when I change

const(T) args

to

const T args

Unfortunately using const T args makes make -f posix.mak unittest fail in several places because std.format doesn't yet correctly handles ranges passed by const. We could define an overload that passes only non-ranges by const. Both cases reduce template bloat in my measurements.

Update: This describes the reduced cases:

class C {}
struct S 
{
    C c; // all calls pass when we qualify this as const
}

void test() {
    S m;
    const S c;
    void f(T...)(const(T) t)
    {
    }
    static assert( __traits(compiles, { f(m, m); }));
    static assert(!__traits(compiles, { f(m, c); }));
    static assert(!__traits(compiles, { f(c, m); }));
    static assert(!__traits(compiles, { f(c, c); }));
}

I'm surprised this isn't covered by Phobos unittests. Is this a compiler bug, @maxhaton @andralex?

@LightBender LightBender added the Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Salvage This PR needs work, but we want to keep it and it can be salvaged. stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants