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

Refactoring fixes for namespace related issues #1216

Merged
merged 2 commits into from
Dec 17, 2023
Merged

Conversation

blairmcg
Copy link
Contributor

  • Static variables and binding refs can employ qualified names to refer to variables that otherwise would be out of scope. When refactorings are moving/creating code, the qualification of names may need to be adjusted in order to bind variables, or may benefit from shortening.
    • Inlined or moving (pushing up or down) methods may need to change static variable qualification
    • Expanding pools/imports is not required when pushing down methods, as pools (and now imports) have always been inherited in Dolphin.
    • Methods can be extracted to component with class var refs if qualified without needing to abstract or add imports
    • Offer the user the option to import or use qualified names when pushing methods up.
    • Abstracting class variables needs to be performed globally, not just in the hierarchy
    • Pulling-up/pushing-down class variables may affect qualified variable references and require global method rewrites
    • When static access is needed because of code movement, abstracting is only needed for private variables. Other access can be qualified.
    • Don't want to generate setters for class constants - writes to these are, of course, compilation errors
  • ChildrenToSiblingsRefactoring, etc, fails to push up methods with custom namespace: Refactorings that move methods need to preserve the package of the original method in case it is compiled into a package default namespace.
  • Fixed a disastrous performance problem in PushUpMethodRefactoring. The algorithm to check sibling supersends is needlessly quadratic in relation to the total number of classes in the superclass hierarchy, so attempting to push up a method to Object takes literally minutes to check the preconditions. This is due to a recursive function using allSubclasses where it should be using subclasses, as it already recurses down the hierarchy.
  • Noticed that the supposed breadth-first traversal of all subclasses is not actually a breadth-first traversal. It visits all the immediate subclasses, but then visits all the subclasses of the first subclass before visiting any of the second subclass. This is a kind of hybrid with depth first. There are consequences in fixing this that need to be worked through, so for the moment just fixing in RBAbstractClass by reverting it to its original allSubclasses implementation (which is actually breadth-first).

Builds on work in 77647b5

Builds on work in 77647b5

Static variables and binding refs can employ qualified names to refer to
variables that otherwise would be out of scope. When refactorings are
moving/creating code, the qualification of names may need to be adjusted
in order to bind variables, or may benefit from shortening.

- Inlined or moving (pushing up or down) methods may need to change
static variable qualification
- Expanding pools/imports is not required when pushing down methods, as
pools (and now imports) have always been inherited in Dolphin.
- Methods can be extracted to component with class var refs if qualified
without needing to abstract or add imports
- Offer the user the option to import or use qualified names when pushing
methods up.
- Abstracting class variables needs to be performed globally, not just in
the hierarchy
- Pulling-up/pushing-down class variables may affect qualified variable
references and require global method rewrites
- When static access is needed because of code movement, abstracting is
only needed for private variables. Other access can be qualified.
- Don't want to generate setters for class constants - writes to these are,
of course, compilation errors
…mespace

Refactorings that move methods need to preserve the package of the original
method in case it is compiled into a package default namespace.

Also fixed a disastrous performance problem in PushUpMethodRefactoring.
The algorithm to check sibling supersends is needlessly quadratic in
relation to the total number of classes in the superclass hierarchy, so
attempting to push up a method to Object takes literally minutes to check
the preconditions. This is due to a recursive function using allSubclasses
where it should be using subclasses, as it already recurses down the
hierarchy.

At which point I noticed that the supposed breadth-first traversal of all
subclasses is not actually a breadth-first traversal. It visits all the
immediate subclasses, but then visits all the subclasses of the first
subclass before visiting any of the second subclass. This is a kind of
hybrid with depth first. There are consequences in fixing this that need
to be worked through, so for the moment just fixing in RBAbstractClass by
reverting it to its original allSubclasses implementation (which is actually
breadth-first).
@blairmcg blairmcg merged commit 93a675d into master Dec 17, 2023
1 of 2 checks passed
@blairmcg blairmcg deleted the blairmcg/refactorvars branch December 17, 2023 12:47
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.

1 participant