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

Fix Issue 9489 - writeln of struct with disabled copy ctor #8055

Merged
merged 1 commit into from
May 12, 2021

Conversation

berni44
Copy link
Contributor

@berni44 berni44 commented May 8, 2021

This fixes the first of the two errors described in issue 9489. However, I'm not sure, if I don't break something else. This is especially true for the ref in the foreach-loop, especially, because when I try to fix the second part too, I get a "Error: symbol _param_0 cannot be ref" pointing to this foreach.

Additionally, this produces a lot of merge conflicts with #8031.

So I'm quite unsure, if this should be merged. What do you think?

@dlang-bot
Copy link
Contributor

dlang-bot commented May 8, 2021

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

Auto-close Bugzilla Severity Description
9489 normal writeln of struct with disabled copy ctor

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#8055"

@berni44
Copy link
Contributor Author

berni44 commented May 8, 2021

This is especially true for the ref in the foreach-loop, especially, because when I try to fix the second part too, I get a "Error: symbol _param_0 cannot be ref" pointing to this foreach.

The testers complain about this too. Any ideas how to handle this more flexible? As far as I know, something like foreach (auto ref ... is not possible.

@berni44
Copy link
Contributor Author

berni44 commented May 9, 2021

Seems to work now. I replaced the foreach over the variadic arguments with a static foreach over an index.

Anyway, please carefully check this, I don't know if this might have unwanted side effects.

@RazvanN7
Copy link
Collaborator

This seems like the correct fix. What happens if you use the normal foreach over an index? Also, could you include the reduced test case present in the bug report?

What part of this issue remains unfixed?

@berni44
Copy link
Contributor Author

berni44 commented May 10, 2021

This seems like the correct fix. What happens if you use the normal foreach over an index?

I'm unsure, what you mean with that.

Also, could you include the reduced test case present in the bug report?

Sure. Soon.

What part of this issue remains unfixed?

typeof(scoped!Foo(1))[10] foos;
foreach (i, ref fi; foos)
    fi.x = i * 10;
writeln(foos); // Error

First of all it has to be

typeof(scoped!Foo(1))[10] foos = void;
foreach (i, ref fi; foos)
    fi.x = cast(int) (i * 10);
writeln(foos); // Error

With that I'll get an static assert error "Scoped[] must be an InputRange" - this seems to be something else.

{
import std.traits : isBoolean, isIntegral, isAggregateType;
import std.utf : UTFException;
auto w = lockingTextWriter();
foreach (arg; args)
static foreach (i; 0 .. args.length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure, what you mean with that.

What happens if you don't use static ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"std/stdio.d(1689): Error: variable i cannot be read at compile time"

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.

Looks good to me

@RazvanN7
Copy link
Collaborator

With that I'll get an static assert error "Scoped[] must be an InputRange" - this seems to be something else.

Yeah, that's unrelated. You might want to update the commit message so that the bot auto-closes the issue.

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label May 10, 2021
@berni44 berni44 changed the title Fix partly Issue 9489 - writeln of struct with disabled copy ctor Fix Issue 9489 - writeln of struct with disabled copy ctor May 10, 2021
@RazvanN7 RazvanN7 added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels May 12, 2021
@dlang-bot dlang-bot merged commit 838da63 into dlang:master May 12, 2021
@ibuclaw
Copy link
Member

ibuclaw commented May 14, 2021

This introduced a regression: https://issues.dlang.org/show_bug.cgi?id=21920

@berni44
Copy link
Contributor Author

berni44 commented May 14, 2021

This introduced a regression: https://issues.dlang.org/show_bug.cgi?id=21920

To be honest, I do not understand that error message ("Error: auto can only be used as part of auto ref for template function parameters"): The offending line is

void writeln(T...)(auto ref T args)

and thus auto is used as part of auto ref. Any ideas?

@CyberShadow
Copy link
Member

auto ref works only with templated functions, because the argument becomes either ref or not depending on how the function called, like with IFTI. If you do writeln!int, then you explicitly instantiate the template, so it's no longer possible to do auto ref.

@berni44
Copy link
Contributor Author

berni44 commented May 14, 2021

auto ref works only with templated functions, because the argument becomes either ref or not depending on how the function called, like with IFTI. If you do writeln!int, then you explicitly instantiate the template, so it's no longer possible to do auto ref.

Sorry, I still don't understand (maybe because I'm not familiar with IFTI - I do not even know what that is). I expect writeln!int to be instantiated as void writeln(int args)?!?

@CyberShadow
Copy link
Member

No. When instantiating the template, the compiler must decide whether to instantiate the auto ref it as (int) or (ref int). Normally it can make that decision based on whether the call argument is an lvalue or not. With explicit instantiation, it does not have that information, hence the error.

void main()
{
	int v;

	void nonref(T)(T t) {}
	nonref(1); // OK
	nonref(v); // OK, by value (a copy occurs)
	alias nonref_int = nonref!int; // OK, void(int)

	void byref(T)(ref T t) {}
	byref(1); // error, requires ref
	byref(v); // OK, by ref
	alias byref_int = byref!int; // OK, void(ref int)

	void autoref(T)(auto ref T t) {}
	autoref(1); // OK, auto ref resolved as non-ref
	autoref(v); // OK, auto ref resolved as ref (no copy)
	alias autoref_int = autoref!int; // error, don't know what to do
}

@berni44
Copy link
Contributor Author

berni44 commented May 15, 2021

	alias autoref_int = autoref!int; // error, don't know what to do

Ah, thanks, now I do understand (and I still think, the message could be improved).

Anyway, I fear, there is no possibility to tell the compiler to use the non-ref version as a default?

@CyberShadow
Copy link
Member

Not that I know of (but I don't know many things).

I suggest reverting the change to fix the regression and add a test until a better solution is found.

@berni44
Copy link
Contributor Author

berni44 commented May 15, 2021

I suggest reverting the change to fix the regression and add a test until a better solution is found.

Go on. As I wrote at the beginning of the PR: I feared something like that, but could not see it.

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