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

Cleanup dcast.d #5197

Merged
merged 2 commits into from Dec 3, 2015
Merged

Cleanup dcast.d #5197

merged 2 commits into from Dec 3, 2015

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Oct 18, 2015

No description provided.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 18, 2015

@yebblies
Copy link
Member

What are you (re-)aligning the comments to? I don't see the point of this when the comments aren't on consecutive lines. Some of these changes (like moving that function) seem to have very little benefit.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 18, 2015

@yebblies As I commented in commit log, the castTo visitors for AddrExp, SymOffExp and DelegateExp are strongly related (they are handling ambiguous function addresses). So placing them in near positions is good.

@WalterBright
Copy link
Member

This is ok with me, except for the repositioning of AddrExp.castTo. That one is just churn. I understand that you wish to move it nearer where it is used, but if we continue down that route the whole source code base will be upended, and my usual nightmare of making it impossible to compare code from one release to the next will be realized.

When doing refactorings, please please keep in mind the affect on diffs.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 22, 2015

Diff? It's not a matter today. When we release a dmd version, we'll forget its previous versions. The maintained release versions and development versions are managed in two distinct breanches (master and stable). If we need to trace some changes, we can use git blame. There's no serious issue.

Ultimately I believe program source code should have no meaningless matters. In this case, implicitConvTo and castTo do very similar things. Therefore so, their visitors' methods should be declared with identical order, if there's no other special thing. From the view, the inconsistent positioning of CastTo.visit(AddrExp e) is just not good.

Why we keep code format style? Because consistent style will help to focus on the code logic. Why I recommend to reorder AddrExp.castTo? Because increasing similarity will highlight the logic difference between implicitConvTo and castTo.

Indeed, it's a little artistic sense. However, I think and believe that making code more meaningfulness will increase code readability and maintainability.

@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 30, 2015

Removed line space changes and repositioning.

`castTo` should not modity `e.type` by the `toBasetype()` result.
Also, `implicitConvTo` should not modify `ImplicitConvTo.t` by the `toBasetype()` result.
@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 3, 2015

Ping to @yebblies and @WalterBright .

@yebblies
Copy link
Member

yebblies commented Dec 3, 2015

Auto-merge toggled on

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 3, 2015

@yebblies Thanks!

yebblies added a commit that referenced this pull request Dec 3, 2015
@yebblies yebblies merged commit 09bbace into dlang:master Dec 3, 2015
@9rnsr 9rnsr deleted the cleanup_dcast branch December 3, 2015 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants