This repository has been archived by the owner on Nov 5, 2022. It is now read-only.
[JENKINS-26481] Patch DGM methods with closure parameters #52
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
so returning the same type makes the patching code above easier
... that we need to rewrite differently.
While running tests, I came across a strange test failure where each(List,Closure) is delegating to each(Iterator,Closure) and results in a cast failure. Obviously, it shuold have resolved to each(Object,Closure) instead. This got me thinking about how Groovy resolves overloaded methods, which is non-trivial as a dynamic language. So in this change I'm changing the code so that Groovy doesn't have to resolve overloaded versions correctly.
The correct signature, which you can check in DefaultGroovyMethods, is to return the 'self' type
…if only once, rather than merely returning the list element.
The suspected cause of the problem is that DGMPatcher is not touching everything it needs to touch, when Groovy runtime is already warm and cached lots of MetaMethod objects. This test, having a name that is alphabetically before CpsTransformerTest, runs before that and populates the cache, exposing the defect in DGMPatcher. Run the test like this: mvn clean test -Dtest=AaaMakeGroovyBusy,CpsTransformerTest
…at elements overriding Object.equals.
... and cast wider net to traverse a bigger graph
…4' into DGMPatcher-Groovy-2-JENKINS-34064 Conflicts: src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java
Reverting cloudbees#38 in this branch.
…nued Conflicts: src/test/groovy/com/cloudbees/groovy/cps/CpsTransformerTest.groovy
Needs more tests for sets and map collect
jglick
reviewed
May 11, 2017
| @@ -14,6 +14,7 @@ | |||
|
|
|||
| <properties> | |||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
| <groovy.version>2.4.11</groovy.version> | |||
There was a problem hiding this comment.
IIUC this will be usable only in 2.61+, right?
| for (Iterator itr = ((ManagedConcurrentLinkedQueue) o).iterator(); itr.hasNext(); ) { | ||
| Object item = itr.next(); | ||
| if (patch(item) != item) { | ||
| LOGGER.log(WARNING, "Can't replace members of ManagedConcurrentLinkedQueue", item); |
There was a problem hiding this comment.
I think you meant
LOGGER.log(WARNING, "Can''t replace members of ManagedConcurrentLinkedQueue: {0}", item);or more nicely
LOGGER.log(WARNING, "Cannot replace members of ManagedConcurrentLinkedQueue: {0}", item);or
LOGGER.log(WARNING, "Can’t replace members of ManagedConcurrentLinkedQueue: {0}", item);| Object item = itr.next(); | ||
| if (patch(item) != item) { | ||
| LOGGER.log(WARNING, "Can't replace members of ManagedConcurrentLinkedQueue", item); | ||
| // TODO would be possible, if necessary, by calling ManagedConcurrentLinkedQueue.Element.remove on *all* elements, then using ManagedLinkedList.add to recreate the entire list |
There was a problem hiding this comment.
possibly copy-paste error in comment
9 tasks
|
The manual changes to |
jglick
suggested changes
May 12, 2017
|
Closing in favor of #56, which now includes tests from here. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JENKINS-26481
Subsumes #51 (which itself subsumes #24).
Note that this is currently a work in progress, adding patched methods one by one, but I wanted to get a PR open for watching the changes as we go.
cc @reviewbybees