Issue 9631 - Error message not using fully qualified name (part 2) by ntrel · Pull Request #7441 · dlang/dmd

ntrel · 2017-12-15T12:25:49Z

@JinShil Following on from #7405.

Disambiguate incompatible type errors.
Add "both operands are of type '%s'" error message case.

The latter saves the programmer time reading the error when there are two types with long qualification.

dlang-bot · 2017-12-15T12:25:51Z

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

Auto-close	Bugzilla	Description
✗	9631	Error message not using fully qualified name when appropriate.

ghost · 2017-12-15T12:55:36Z

src/dmd/expression.d

+        }
+        else if (e1.type.equals(e2.type))
+        {
+            error("incompatible types for ((%s) %s (%s)): both operands are of type '%s'",


I think it would be better to denote that the operator %s cannot be used for two variables of type %s.
But maybe in another PR ?

Reworded in f73a72b.

Disambiguate incompatible type errors. Add "both operands are of type '%s'" error message.

JinShil · 2017-12-15T14:43:22Z

src/dmd/expression.d

        if (e1.op == TOKtype || e2.op == TOKtype)
        {
-            error("incompatible types for ((%s) %s (%s)): cannot use '%s' with types", e1.toChars(), Token.toChars(thisOp), e2.toChars(), Token.toChars(op));
+            error("incompatible types for ((%s) %s (%s)): cannot use '%s' with types",


I think we can change ((%s) %s (%s)) to `(%s) %s (%s)`. That is, lose the outer parentheses and surround the expression in back ticks.

Also, those single quotes should be backticks.

I agree, but I was just following the existing format for these errors. I'll do as you suggest for the new error added in this PR, but the other ones can be fixed in a separate PR.

JinShil · 2017-12-15T14:47:51Z

test/fail_compilation/fail3.d

-        a.x = x;
-        a.y = y;
-        return a;
-    }


Why was this removed?

It doesn't seem relevant to the test - should I restore it?

Yes, it does seem unnecessary, but I don't think it belongs in this PR.

OK, restored.

JinShil · 2017-12-15T14:48:30Z

test/fail_compilation/bug9631.d

+    F x = tem!().F();
 }

-void main()


Why the refactoring of this test case? There should only be changes to the error messages.

I thought it was a bit simpler whilst still essentially testing the same thing. But I suppose it may be worth having both the two template version and the new changes.

Restored the two template version.

ghost · 2017-12-15T16:42:22Z

src/dmd/expression.d

+        }
+        else if (e1.type.equals(e2.type))
+        {
+            error("invalid operation for `(%s) %s (%s)` with both operands of type `%s`",


the error message is still bad. My first review comment was more accurate. Maybe:

"the %s operator is not supported for two operands of type %s ((%s) %s (%s))".

I'm not sure there's much difference, but if necessary I'll update it.

ntrel · 2017-12-16T12:41:34Z

I've reverted the rewording of the new error message, so it now just follows the existing format, and does not use backticks. The backtick change should be done in a separate PR as it affects about 8 error messages in dmd, I could do it if this gets merged soon.

JinShil · 2017-12-16T13:34:12Z

src/dmd/expression.d

        if (e1.op == TOKtype || e2.op == TOKtype)
        {
-            error("incompatible types for ((%s) %s (%s)): cannot use '%s' with types", e1.toChars(), Token.toChars(thisOp), e2.toChars(), Token.toChars(op));
+            error("incompatible types for ((%s) %s (%s)): cannot use '%s' with types",


This error message doesn't seem to have a test case.

@JinShil What about fail_compilation/fail54.d and fail_compilation/ice8511.d?

Sorry, I realize that error message didn't actually change.

My mistake

ntrel · 2017-12-18T15:40:33Z

Thanks for reviewing.

The backtick change should be done in a separate PR

See #7464.

ntrel force-pushed the qual-incompatible branch from 966bd8a to 41078c6 Compare December 15, 2017 12:41

ntrel requested a review from WalterBright as a code owner December 15, 2017 12:41

ghost reviewed Dec 15, 2017

View reviewed changes

ntrel force-pushed the qual-incompatible branch from 41078c6 to e233e2e Compare December 15, 2017 13:15

Issue 9631 - Error message not using fully qualified name (part 2)

cf01f68

Disambiguate incompatible type errors. Add "both operands are of type '%s'" error message.

ntrel force-pushed the qual-incompatible branch from e233e2e to cf01f68 Compare December 15, 2017 14:19

JinShil reviewed Dec 15, 2017

View reviewed changes

Restore original two template test

c33df42

ghost reviewed Dec 15, 2017

View reviewed changes

Restore unnecessary make2()

51b0dce

ntrel force-pushed the qual-incompatible branch from f73a72b to 51b0dce Compare December 16, 2017 12:38

JinShil previously requested changes Dec 16, 2017

View reviewed changes

JinShil added the Merge:auto-merge label Dec 16, 2017

dlang-bot merged commit 6504493 into dlang:master Dec 16, 2017

ntrel deleted the qual-incompatible branch December 18, 2017 14:51

Uh oh!

Conversation

ntrel commented Dec 15, 2017

Uh oh!

dlang-bot commented Dec 15, 2017

Bugzilla references

Uh oh!

ghost Dec 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntrel commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntrel commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost Dec 15, 2017 •

edited by ghost

Loading

JinShil Dec 15, 2017 •

edited

Loading