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

Partial fix issue 19294 - Support for array operations with Complex! is incomplete #13154

Merged
merged 3 commits into from Nov 8, 2021

Conversation

sorin-gabriel
Copy link
Contributor

…plete (partial fix, only for operands of the same type)

The git diff isn't very helpful here, as the only real changes are the removal of the return statement and the outer if condition. The if condition is redundant and the early return caused the compiler to throw errors for array operations like T[] op T or T op T[]

This is a partial fix for the issue, as T[] op U, U op T[] or T[] op U[] still aren't covered, even when op overload is defined.

@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 10, 2021

Thanks for your pull request and interest in making D better, @sorin-gabriel! 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
19294 normal Support for array operations with Complex! is incomplete

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 + dmd#13154"

@thewilsonator
Copy link
Contributor

Please add the test case(s) from the bug report.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

since this is a partial fix, please remove the word fix from the commit message of the first commit since this doesn't completely fix the issue. (the bot should update its message to reflect that fact).

@@ -0,0 +1,26 @@
import std.stdio;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed

@@ -0,0 +1,26 @@
import std.stdio;
import std.complex;
Copy link
Contributor

Choose a reason for hiding this comment

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

replace this with an inline definition of Complex the test suite is not allowed t depend on the standard library. a simple

struct Complex(T)
{
     T re, im;
    // Methods (i.e. opBinary and opBinaryRight) + alias this
    // and anything else necessary to reproduce the bug
}

should suffice, no need for the body of the methods, just a return this; should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is not with Complex, but with any user-defined type that has an opBinary.

import std.stdio;
import std.complex;

void main()
Copy link
Contributor

Choose a reason for hiding this comment

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

test functions in compilable should not be main functions.

@RazvanN7
Copy link
Contributor

You still need to rebase to get rid of "Fix" from the commit message.

@sorin-gabriel sorin-gabriel changed the title Fix issue 19294 - Support for array operations with Complex! is incom… Partial fix issue 19294 - Support for array operations with Complex! is incomplete Oct 22, 2021
@dlang-bot dlang-bot merged commit cc4e83b into dlang:master Nov 8, 2021
sorin-gabriel added a commit to sorin-gabriel/dmd that referenced this pull request Nov 8, 2021
@sorin-gabriel
Copy link
Contributor Author

Wanted to click "View details", but clicked on "Revert" instead. Please ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants