Refactor FunDecl semantic by thewilsonator · Pull Request #8609 · dlang/dmd

thewilsonator · 2018-08-24T02:55:54Z

I sincerely apologise for the diff size, and would recommend using something other than the regular diff algorithm as it logically consists of a small addition combined with a massive deletion, and a massive addition, not lots of additions and deletions. Does anyone know how to use --patience or --histogram with GitHub?

dlang-bot · 2018-08-24T02:55:54Z

Thanks for your pull request and interest in making D better, @thewilsonator! 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 "master + dmd#8609"

JinShil · 2018-08-24T04:15:17Z

Would be better if each new function were its own commit, or even better its own PR. But a few global comments before I even attempt to review this: 1) All new functions should have full DDoc headers with Params and Returns where applicable. Follow the style guide at https://dlang.org/dstyle.html#phobos_documentation 2) make any new functions private if they don't need to be exposed to other modules. 3) consider if any of the functions should be methods of the class instead of free functions in semantic. 4) void invariant(): I would prefer if function names always began with verbs. What is that function doing? Same with nestedFuncLit().

thewilsonator · 2018-08-24T04:39:25Z

Will do func per commit.

Almost all the function are void function()s. I'm trying improve the docs with those, but will do.
The whole struct is private do the functions need to be as well?
? they are all part of a private struct.
Ya I'm having a hard time describing what they actually do. I've been staring at them for too long. Will take a break and have a look with fresh eyes and a rested mind.

JinShil · 2018-08-24T04:53:44Z

? they are all part of a private struct.

Sorry, I didn't look at it in detail. It's hard to see the surround scope in these diffs. It's not a concern if the scope is private.

thewilsonator · 2018-08-24T04:55:06Z

Yeah, hopefully will be more obvious when I do it 1 commit per function.

WalterBright · 2018-08-24T09:17:44Z

Sorry, this is pretty much impossible to review, and no description of what is being done here. Try multiple PRs, each with a smallish chunk (not multiple commits to one PR). Is there a reason to combine all these into one PR?
BTW, this is failing all tests. Doing smaller PRs means the mistake is FAR FAR easier to find.

RazvanN7 · 2018-08-24T15:49:05Z

src/dmd/semantic3.d


-                assert(funcdecl.type == f || (funcdecl.type.ty == Tfunction && f.purity == PURE.impure && (cast(TypeFunction)funcdecl.type).purity >= PURE.fwdref));
-                f = cast(TypeFunction)funcdecl.type;
+    void semantic()


Naming this function semantic is really confusing. In dmd there are 3 semantic stages named bluntly dsymbolSemantic, semantic2 and semantic3. Although the function is defined in the file semantic3, on caller site it is not obvious to the reader which of the semantic passes it is calling. I would recommend renaming it doSemantic3 or semantic3Impl or whatever that makes it obvious that it's the invocation of semantic3

RazvanN7 · 2018-08-24T15:53:22Z

src/dmd/semantic3.d

-                sym.loc = funcdecl.loc;
-                sym.endlinnum = funcdecl.endloc.linnum;
-                sc2 = sc2.push(sym);
+private struct FuncDeclSem


Is this struct really needed? Why not just free functions?

RazvanN7

Making smaller PRs is the way to go here and I suggest first breaking the visit(FuncDeclaration) method into smaller functions. After all those PR's are merged you should try submitting the PR with the creation of the struct FuncDeclSem because it will be more obvious if the struct is needed or not. Smaller refactoring PRs are easy to review and merge.

thewilsonator · 2018-08-26T01:48:37Z

The struct is absolutely required, otherwise each function call will take a ridiculous number of arguments. The struct will hold each used local variable of visit(FuncDeclaration), and there are by my count 13 of them.

ibuclaw · 2018-08-26T20:53:17Z

src/dmd/semantic3.d

    }
 }
+
+private struct FuncDeclSem3


Maybe you should instead move this to a new module which encapulates running semantic on FuncDeclaration, you could also move functionSemantic and functionSemantic3 there too.

module dmd.funcsemantic; extern(C++) bool functionSemantic(FuncDeclaration fd); extern(C++) bool functionSemantic3(FuncDeclaration fd); private struct Semantic3Function // Or whatever you insist on naming it. { FuncDeclaration funcdecl; }

The visitor entry in semantic3.d could then just call functionSemantic3.

Actually, the tangle is even worse than I thought.

We have the following methods that do "semantic" on a function.

DsymbolSemanticVisitor::visit(FuncDeclaration)

DsymbolSemanticVisitor::funcDeclarationSemantic(FuncDeclaration)

FuncDeclaration::functionSemantic

FuncDeclaration::functionSemantic3

Semantic3Visitor::visit(FuncDeclaration)

I revise my suggestion, they should probably go into two separate leaf modules - funcsemantic and funcsemantic3 - each with one public entry point named accordingly.

You're saying fold Semantic3Visitor::visit(FuncDeclaration) out of https://github.com/dlang/dmd/blob/master/src/dmd/func.d#L391 so that it calls the function directly? Sure, but I think it would be better suited to a later PR. I want to get this refactored to a comprehendible state so I can fix 14246 with hopefully a bit more finesse than last time it was tried.

I've half a mind to (eventually) move all the semantic analysis modules to a separate directory, src/dmd is mighty cluttered.

I think this is ready to be merged, not sure why buildkite is failing.

WalterBright · 2018-08-27T20:18:07Z

src/dmd/semantic3.d

+            }
+        }
+    }
+}


I see the point of factoring out checkInContractOverrides() into a separate function, but I don't see the point of creating FuncDeclSem3?

It will hold local variables of visit(FuncDeclaration) so that there won't be heaps of arguments to the function.

We already have Scope for that.

thewilsonator · 2018-09-01T23:50:27Z

Can we move forward with this? I'm not sure why build kite is failing though.

jacob-carlborg · 2018-09-03T06:03:04Z

src/dmd/semantic3.d

        }

        uint oldErrors = global.errors;
+        auto fds = FuncDeclSem3(funcdecl,sc);


Can this be made const or are we expecting it to change?

It will eventually. e: change that is. It will hold the locals vars of the function which all do.

ibuclaw · 2018-09-05T21:51:40Z

Why was this merged without waiting for all reviewers to acknowledge the responses to their questions?

jacob-carlborg · 2018-09-06T05:45:52Z

Someone thought two approvals were good enough?

WalterBright · 2018-09-06T07:43:47Z

This is apparently the start of a major refactoring. I don't know why it is being done. What is it intended to accomplish? How will the code get better? Until this is all clear and agreed upon, we should not be proceeding with it.

For example, I've provided lists before of goals of refactoring. As far as I can see, this addresses none of them.

RazvanN7 · 2018-09-06T07:58:55Z

@ibuclaw This is just the start of the refactorization and it is not divergent with your suggestion. The purpose of this refactorization is to break the semantic3 for FuncDeclarations into smaller-documented functions. Your suggestion can be implemented in a future PR

@WalterBright The goal of this refactoring is to make semantic3::visit(FuncDeclaration) more readable as it has >1000 lines of code. I have been working with that code myself and it is really hard to try and isolate the code of interest in that giant chunk. This PR (and subsequent ones) will refactor the logic into separate function that are documented. This will make it easier to read and understand what the code does.

ibuclaw · 2018-09-11T07:02:32Z

As per https://github.com/dlang/dmd/pull/8609/files#discussion_r213101229 and #8609 (comment), moving from a member function of one type to a member function of another is not improving encapsulation. At least my suggestion of putting all moved code into a free function in a new module should have been followed (unless me and @WalterBright disagree there, of course).

thewilsonator · 2018-09-11T07:16:57Z

You are right, but the point of this and the next 24ish PRs is not to improve encapsulation but to improve comprehensibility, 1100 lines is too much for me to make functional changes in and I suspect too much for reviewers as well. PRs that follow after that can deal with encapsulation.

ibuclaw · 2018-09-12T16:51:34Z

However moving the code to another place in the same module does not improve comprehensibility. You're better off hitting two birds with one stone here and just moving them to a new module from the start.

thewilsonator · 2018-09-12T23:40:04Z

However moving the code to another place in the same module does not improve comprehensibility.

Oh, but it does. Going from 1100 lines of almost completely undocumented code, to ~300 of documented code at a high level leads to a massive increase in the ability to comprehend what the code is doing. Or you have a different definition of comprehensibility than I do.

Splitting out code to a new module can come later.

RazvanN7 reviewed Aug 24, 2018

View reviewed changes

RazvanN7 requested changes Aug 24, 2018

View reviewed changes

thewilsonator force-pushed the refactor-funcsem branch from 2f90a80 to 2f3c76d Compare August 26, 2018 01:41

[FuncDeclSem3 refactor] in contract overrides

3741497

thewilsonator force-pushed the refactor-funcsem branch from 2f3c76d to 3741497 Compare August 26, 2018 01:48

ibuclaw reviewed Aug 26, 2018

View reviewed changes

WalterBright reviewed Aug 27, 2018

View reviewed changes

jacob-carlborg reviewed Sep 3, 2018

View reviewed changes

jacob-carlborg approved these changes Sep 3, 2018

View reviewed changes

RazvanN7 approved these changes Sep 3, 2018

View reviewed changes

RazvanN7 added the Merge:auto-merge label Sep 5, 2018

dlang-bot merged commit d6ad81d into dlang:master Sep 5, 2018

thewilsonator mentioned this pull request Sep 6, 2018

[Semantic3 Refactor] setupScope #8668

Closed

dkorpel mentioned this pull request Jul 2, 2025

Ddoc comments to semantic3.d #21499

Merged

Uh oh!

Comments

Conversation

thewilsonator commented Aug 24, 2018

Uh oh!

dlang-bot commented Aug 24, 2018

Bugzilla references

Testing this PR locally

Uh oh!

JinShil commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewilsonator commented Aug 24, 2018

Uh oh!

JinShil commented Aug 24, 2018

Uh oh!

thewilsonator commented Aug 24, 2018

Uh oh!

WalterBright commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Aug 26, 2018

Uh oh!

ibuclaw Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Sep 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Sep 5, 2018

Uh oh!

jacob-carlborg commented Sep 6, 2018

Uh oh!

WalterBright commented Sep 6, 2018

Uh oh!

RazvanN7 commented Sep 6, 2018

Uh oh!

ibuclaw commented Sep 11, 2018

Uh oh!

thewilsonator commented Sep 11, 2018

Uh oh!

ibuclaw commented Sep 12, 2018

Uh oh!

thewilsonator commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JinShil commented Aug 24, 2018 •

edited

Loading

RazvanN7 left a comment •

edited

Loading

ibuclaw Aug 26, 2018 •

edited

Loading

ibuclaw Aug 26, 2018 •

edited

Loading

thewilsonator Sep 3, 2018 •

edited

Loading