Skip to content

Comments

Avoid eager error message allocation with functionResolve#8232

Merged
RazvanN7 merged 1 commit intodlang:stablefrom
ntrel:resolve-msg
May 19, 2018
Merged

Avoid eager error message allocation with functionResolve#8232
RazvanN7 merged 1 commit intodlang:stablefrom
ntrel:resolve-msg

Conversation

@ntrel
Copy link
Contributor

@ntrel ntrel commented May 8, 2018

  • Don't set failMessage with functionResolve until it's needed.

This is a less drastic fix than #8216. In particular, this pull does not cause problems for #7584. The latter pull is a good example of why functionResolve cannot just return a parameter index and create the error message on the caller side.

@dlang-bot
Copy link
Contributor

dlang-bot commented May 8, 2018

Thanks for your pull request and interest in making D better, @ntrel! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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 fetch digger
dub run digger -- build "stable + dmd#8232"

@JinShil
Copy link
Contributor

JinShil commented May 8, 2018

Since this is a critical regression, please target stable instead of master.

@JinShil
Copy link
Contributor

JinShil commented May 9, 2018

@Geod24 Could you please test this locally for your use case and let us know if it resolves the issue you encountered?

@ntrel ntrel changed the base branch from master to stable May 9, 2018 10:04
const(char)* failMessage;
functionResolve(&m, s, loc, sc, tiargs, tthis, fargs, &failMessage);
functionResolve(&m, s, loc, sc, tiargs, tthis, fargs, null);
auto orig_s = s;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the extra reference? You can just use s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, ok missed that

@RazvanN7
Copy link
Contributor

ping @ntrel . What's the status on this?

@RazvanN7 RazvanN7 merged commit 463160b into dlang:stable May 19, 2018
@ntrel ntrel deleted the resolve-msg branch May 19, 2018 13:21
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.

4 participants