Skip to content

Comments

Cleanup dcast.d#5197

Merged
yebblies merged 2 commits intodlang:masterfrom
9rnsr:cleanup_dcast
Dec 3, 2015
Merged

Cleanup dcast.d#5197
yebblies merged 2 commits intodlang:masterfrom
9rnsr:cleanup_dcast

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
Contributor

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 9rnsr force-pushed the cleanup_dcast branch 2 times, most recently from 3f1bf84 to 4b90faa Compare November 30, 2015 02:08
@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 30, 2015

Removed line space changes and repositioning.

9rnsr added 2 commits December 3, 2015 15:48
`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
Contributor

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

Development

Successfully merging this pull request may close these issues.

3 participants