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.

Sign up for GitHub

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

Jump to bottom

Fix Issue 20616 - Error: undefined identifier __dollar #13130

Merged
merged 1 commit into from Dec 13, 2021
Merged

Fix Issue 20616 - Error: undefined identifier __dollar #13130

merged 1 commit into from Dec 13, 2021

Conversation

dorianverna17
Copy link
Contributor

@dorianverna17 dorianverna17 commented Oct 5, 2021 •
edited

The link for the Issue 20616:
https://issues.dlang.org/show_bug.cgi?id=20616

@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 5, 2021 •
edited

Thanks for your pull request and interest in making D better, @dorianverna17! 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
✓ 20616 enhancement Error: undefined identifier __dollar

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

@maxhaton
Copy link
Member

maxhaton commented Oct 5, 2021

There are two patches in one here, the former was rejected in #3079 from what I can tell.

maxhaton
maxhaton reviewed Oct 5, 2021
View reviewed changes
src/dmd/opover.d Outdated Show resolved Hide resolved
maxhaton
maxhaton previously requested changes Oct 5, 2021
View reviewed changes
Copy link
Member

@maxhaton maxhaton left a comment

Choose a reason for hiding this comment

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

See previous comments

@dorianverna17 dorianverna17 changed the title Issue 20616 Issue 20616 - https://issues.dlang.org/show_bug.cgi?id=20616 Oct 5, 2021
@dorianverna17 dorianverna17 changed the title Issue 20616 - https://issues.dlang.org/show_bug.cgi?id=20616 Issue 20616 Oct 5, 2021
RazvanN7
RazvanN7 requested changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@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.

Can you please explain what is the problem (as in what is currently causing the bug) and what is the general idea of your solution?

src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
@RazvanN7 RazvanN7 changed the title Issue 20616 Fix Issue 20616 - Error: undefined identifier __dollar Oct 11, 2021
@RazvanN7
Copy link
Contributor

ping @dorianverna17

RazvanN7
RazvanN7 reviewed Dec 1, 2021
View reviewed changes
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
ljmf00
ljmf00 reviewed Dec 5, 2021
View reviewed changes
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
RazvanN7
RazvanN7 reviewed Dec 6, 2021
View reviewed changes
test/fail_compilation/fail20616.d Outdated Show resolved Hide resolved
src/dmd/opover.d Outdated Show resolved Hide resolved
thewilsonator
thewilsonator reviewed Dec 6, 2021
View reviewed changes
src/dmd/expressionsem.d
@@ -2795,6 +2795,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
exp.error("undefined identifier `%s`, did you mean %s `%s`?", exp.ident.toChars(), s2.kind(), s2.toChars());
else if (const p = Scope.search_correct_C(exp.ident))
exp.error("undefined identifier `%s`, did you mean `%s`?", exp.ident.toChars(), p);
else if (exp.ident == Id.dollar)
exp.error("undefined identifier `$`");
Copy link
Contributor

Choose a reason for hiding this comment

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

$ is not really an identifier (I know it is implemented as such) its an operator

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the error message is fine this way. It behaves as an identifier in this situation.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Dec 9, 2021

Please add a commit that has "Fix Issue 20616" in the commit message, so that dlang-bot picks this PR and links it to the associated bugzilla entry. Upon merger, the bot will automatically close the issue.

@dorianverna17
Fix Issue 20616
8ea2a3d 
Issue-20616: Modified commit PR for this issue, added supplemental error call to solve issue

Issue-20616: modified PR according to the review

Issue-20616: made small changes in the PR according to the review

Modified PR according to the review

modified PR according to the review

Issue-20616: modified output message in expressionsem.d and fail compilation test and condition in opover.d
@dlang-bot dlang-bot merged commit 0db8e0a into dlang:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljmf00 ljmf00 ljmf00 left review comments

@thewilsonator thewilsonator thewilsonator left review comments

@RazvanN7 RazvanN7 RazvanN7 approved these changes

@maxhaton maxhaton Awaiting requested review from maxhaton

@MoonlightSentinel MoonlightSentinel Awaiting requested review from MoonlightSentinel

Assignees
No one assigned
Labels
auto-merge Enhancement
Projects
None yet
Milestone
No milestone
7 participants
@dorianverna17 @dlang-bot @maxhaton @RazvanN7 @ljmf00 @thewilsonator @MoonlightSentinel