- 
                Notifications
    
You must be signed in to change notification settings  - Fork 163
 
fix: process deep-cloned entities in a queue for GizmoSolutionCloner #1849
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
fix: process deep-cloned entities in a queue for GizmoSolutionCloner #1849
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're already improving the code, I'm making some further improvement suggestions of my own. Reading this code is IMO harder than it could be.
Also, please make sure you work with up-to-date main, because CI is currently failing due to that.
        
          
                .../java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...est/java/ai/timefold/solver/core/impl/domain/solution/cloner/AbstractSolutionClonerTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...i/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java
          
            Show resolved
            Hide resolved
        
      8243a82    to
    3367128      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please review the Sonar comments.
- Previously, when a class is deep-cloned, a method that constructs and fully initializes a clone of that class is called. This has the potential to cause a StackOverflowException when a deep-cloned class refers to many other deep-cloned classes, such as in chained models. - Now, when a field is deeped cloned, it adds the field initializer to a cloneQueue, and that cloneQueue can only be processed for the first entity being cloned. As such, stack-height does not grow indefinitely. - Perform various cleanups in the GizmoSolutionCloner - Change method `isInvalid` to `getIsInvalid` so the GizmoSolutionCloner recongize it as the getter of the corresponding field.
3367128    to
    93410ca      
    Compare
  
    
          
 | 
    



Previously, when a class is deep-cloned, a method that constructs and fully initializes a clone of that class is called. This has the potential to cause a StackOverflowException when a deep-cloned class refers to many other deep-cloned classes, such as in chained models.
Now, when a field is deeped cloned, it adds the field initializer to a cloneQueue, and that cloneQueue can only be processed for the first entity being cloned. As such, stack-height does not grow indefinitely.
Perform various cleanups in the GizmoSolutionCloner
Change method
isInvalidtogetIsInvalidso the GizmoSolutionCloner recongize it as the getter of the corresponding field.