inline.d: remove copy/pasta, fix obtuse logic

[WalterBright]

Trying to understand how this worked revealed a bunch of copy/pasta code with mistakes, false assumptions, and misleading logic. Straightened it out.

[9rnsr]

Hard to review. Please separate commits to:

1. renaming variables
2. reduction of copy/pasta code
3. logic improvements

[WalterBright]

I appreciate the sentiment, but I think you'll find in this case that one doesn't work without the other, because the logic had to be fixed to make the refactoring work, and vice versa.

[9rnsr]

As far as I see, there's bunch of unrelated changes to the "logic improvement" - updating copyright comment, variable renaming (result -> sresult), int to bool conversion, changing scanVar return type to void. Why do you think they should be done at once?

I really know, such the code refactoring is - even if it's trivial renaming - necessary for further improvements. But such the squashed one big commit is unfriendly for the reviewers.

If you want to do multiple things in one PR, they should be separated in the same number of commits. I think considering to the reviewability is the duty of contributors.

[WalterBright]

Consider things in proportion. The result->sresult is one character on one line (all the other lines with result were replaced for other reasons). This is not 500 lines of refactoring and 10 lines of bug fixing. scanVar returning void is necessary to fix a bug where eresult erroneously depended on it, and all the logic that set the return value was utterly broken and had to be removed. The int to bool change affected one line of code in logic that was redone anyway.

I know I've complained to you about refactorings mixed up with bug fixes. I have good reason to believe that those instances did impede review, due mostly to the size of them. I think you'll agree, though, that this is not the case here, and changing the file comment at the top hardly impedes understanding.

[yebblies]

Walter, you made the changes so of course you understand them. Others reading your PRs do not immediately know which parts are related, and which aren't. Splitting up commits is so much easier than arguing about it.

[9rnsr]

Of course the refactoring changes are so small in this PR. But it was actually obstacle to my quick reviewing.

And I've reviewed this PR beyond the obstacles. The total change looks good to me.

[9rnsr]

After the review, I've found four improvements for the inline.d code.

1. Extract inlineScan call on CallExp.e1 and arguments out of visitCallExp function.
   -> I'm not sure why it's necessary to "logic improvement", but not a problem.
2. Add asStatements parameter to visitCallExp function, and then remove one copy/pasta code block.
   -> Looks good.
3. Move the position of "verbose" message printing
   -> It's another removing of copy/pasta code. Good.
4. scanVar returns void, and expandInline will return null when ps !is null
   -> "scanVar returning void is necessary to fix a bug where eresult erroneously depended on it, and all the logic that set the return value was utterly broken and had to be removed. "

If you had separated those changes (and refactorings) into independent commits, the review would have quickly finished. Especially, the #4 is difficult to understand from the diff view. I'd really prefer to record it as a commit log in the git history.

[WalterBright]

They are connected, @yebblies. The changes to visitCallExp caused scanVar's setting of eresult to break things. How was I supposed to make that a separate commit? Replace the return value with null and then ignore it? If I did that, I'd get (properly) dinged in code review.

Notice that visitCallExp now handles both inlining for statements and inlining for expressions. Trying to keep the original logic in scanVar did not work with that. Having visitCallExp do double duty now required rewriting the logic that triggered the verbose message printing, moving it outside the { } solved that.

The logic before was almost impossible to follow, particularly how eresult was set, because the save/restore of it implied recursion when it actually worked quite differently, and the save/restore was a hack somebody put in to work around scanVar setting eresult. Both functions need to be redone to untangle it.

I changed result to sresult because I tired of all the false positives when grepping for result. It's still only one line of additional change. I don't believe one line with a one character change causes anyone difficulty.

Again, it's about proportion. This is NOT a 500 line refactoring. It's a decent shrink, more than the line count suggests because I added many lines of comments.

This PR is a necessary step towards fixing the inlining issues everyone gripes about. Please pull it. If there's anything else you don't understand about it, I'll be happy to explain.

[WalterBright]

BTW, the git diff view makes things look more complex than they are. Just use that as a guide to where the changes are, and bring up the file separately, and you'll see that the logic flows better and is much simpler. You'll also find that searching for sresult is much more practical than searching for result - it will save time reviewing, not cost :-)

[9rnsr]

I wish you'll open a more reviewer-friendly PR in the next time.

[9rnsr]

Auto-merge toggled on

[CyberShadow]

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15296