Skip to content

Comments

Cleanup hasOverloads#5198

Merged
ibuclaw merged 1 commit intodlang:masterfrom
9rnsr:cleanup_hasOverloads
Feb 15, 2016
Merged

Cleanup hasOverloads#5198
ibuclaw merged 1 commit intodlang:masterfrom
9rnsr:cleanup_hasOverloads

Conversation

@9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Oct 18, 2015

Remove Dsymbol.hasOverloads() because it's confusing method.

@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch from 934c08e to 9cf04ef Compare October 18, 2015 12:56
@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 18, 2015

This front-end cleanup is necessary for:

https://trello.com/c/ihKhGeiu/115-reg2-031-forward-reference-to-inferred-return-type-of-function-call-when-using-auto-return-type

yebblies
yebblies reviewed Oct 18, 2015
View reviewed changes
src/expression.d Outdated
Copy link
Contributor

@yebblies yebblies Oct 18, 2015

Choose a reason for hiding this comment

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

"Won't be generated" or "Isn't generated"

@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch 2 times, most recently from 4fb71d0 to 221d058 Compare October 18, 2015 13:16
@9rnsr 9rnsr mentioned this pull request Oct 19, 2015
Merged
3 tasks
@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch from 221d058 to 46700a6 Compare November 16, 2015 00:22
dnadlinger
dnadlinger reviewed Nov 16, 2015
View reviewed changes
src/expression.d Outdated
Copy link
Contributor

@dnadlinger dnadlinger Nov 16, 2015

Choose a reason for hiding this comment

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

What's with the commented out assert?

Copy link
Contributor Author

@9rnsr 9rnsr Nov 16, 2015

Choose a reason for hiding this comment

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

It was a junk. Removed.

@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch from 46700a6 to a1e8b14 Compare November 16, 2015 06:34
@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch 2 times, most recently from 2ce3a57 to 20f963b Compare December 3, 2015 06:53
@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 3, 2015

Ping to @yebblies and @klickverbot .

In TypeStruct (or TypeClass) dotExp(), it resolves a form e.ident to an actual expression. And if ident is a function, it's not yet known whether it has overloads or not. In other words, following semantic analysis should assume that the function would have some overloads. That's　why we should set DotVarExp.hasOverloads to true if d is a FuncDeclaration.

The logic will also fit to TypeExp.semantic and ScopeExp.semantic.

@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch from 20f963b to 8b83389 Compare December 3, 2015 14:30
@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch 3 times, most recently from c8d03fb to 6c39247 Compare February 5, 2016 14:38
@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch 2 times, most recently from 6831881 to 064bb1a Compare February 14, 2016 12:26
@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 14, 2016

Updated to fix all settings of (Dsymbol|Var|DotVar|SymOff|Delegate)Exp.hasOverloads fields.

Usually, when a function symbol appears in expression, e.g. func.d(), &func etc, the func has overload candidates. So, usually hasOverloads field should be set to true until it's resolved. Changing their default arguments to true, and supply false explicitly around internal code generations.

Now this PR is shared by #2130 and #5202.

@9rnsr 9rnsr mentioned this pull request Feb 14, 2016
Closed
ibuclaw
ibuclaw reviewed Feb 15, 2016
View reviewed changes
src/expression.d
bool hasOverloads;

extern (D) this(Loc loc, Dsymbol s, bool hasOverloads = false)
extern (D) this(Loc loc, Dsymbol s, bool hasOverloads = true)
Copy link
Member

@ibuclaw ibuclaw Feb 15, 2016

Choose a reason for hiding this comment

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

Maybe it's better just to force hasOverloads to be explicitly passed?

Copy link
Contributor Author

@9rnsr 9rnsr Feb 15, 2016

Choose a reason for hiding this comment

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

I don't think so. Half of hasOverloads would need to be set to false for VarDeclarations. For them, explicit argument passing is just verbose. The default argument true works for good default behavior.

@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch from 064bb1a to 2d5290e Compare February 15, 2016 15:10
@9rnsr
Fix hasOverloads field settings to correctly handle overloaded functi…
0364953 
…on names

Grep-ed `new (Dsymbol|Var|DotVar|SymOff|Delegate)Exp` and has checked them all.

Usually, when a function symbol appears in expression, e.g. `func.d()`, `&func` etc, the `func` has overload candidates. So, usually `hasOverloads` field should be set to `true` until it's resolved. Changing their default arguments to `true`, and supply `false` explicitly around internal code generations.
@9rnsr 9rnsr force-pushed the cleanup_hasOverloads branch from 2d5290e to 0364953 Compare February 15, 2016 15:15
@ibuclaw
Copy link
Member

ibuclaw commented Feb 15, 2016

Auto-merge toggled on

ibuclaw added a commit that referenced this pull request Feb 15, 2016
@ibuclaw
Merge pull request #5198 from 9rnsr/cleanup_hasOverloads
3d726b1 
Cleanup hasOverloads
@ibuclaw ibuclaw merged commit 3d726b1 into dlang:master Feb 15, 2016
@ibuclaw
Copy link
Member

ibuclaw commented Feb 15, 2016

Ok, well on the whole I have nothing against this.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 16, 2016

Thanks!

@9rnsr 9rnsr deleted the cleanup_hasOverloads branch February 16, 2016 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@9rnsr @ibuclaw @dnadlinger @yebblies