-
Notifications
You must be signed in to change notification settings - Fork 7
Fix for #103 #124
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 for #103 #124
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.
Please add one more test case similar to testNonInternalAPI4() but varying execution modes.
|
||
public void testNonInternalAPI4() throws Exception { | ||
helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()", | ||
Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false, |
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.
@saledouble This should be ordered, correct?
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.
@saledouble I'm confused as to why the the ordering is UNORDERED
. Do you know why? I am wondering if it is the case that we determine ordering upon a terminal operation and when we think there's none, we are just using some default value. What do you think?
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.
If the test case is
class A {
Stream<Object> m() {
Stream<Object> stream = new HashSet<>().stream();
return stream;
}
@EntryPoint
void n() {
Stream<Object> s = m();
s.sorted().distinct().count();
}
}
the ordering is 'UNORDERED' and the project throws an exception requiring terminal operations.
If the test case is
class A {
@EntryPoint
void n() {
Stream<Object> s = new HashSet<>().stream();;
s.sorted().distinct().count();
}
}
the ordering is 'ORDERED' and the project doesn't throw an exception.
Then, I found a sentence from https://docs.oracle.com/javase/8/docs/api/java/util/stream/package-summary.html :
"When the terminal operation is initiated, the stream pipeline is executed sequentially or in parallel depending on the orientation of the stream on which it is invoked."
I think the project doesn't find the terminal operations, so the stream pipeline has never been executed.
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.
If you replace HashSet()
with ArrayList
, is it still UNORDERED
? That would answer whether it is just accepting a default value.
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.
Do you have any insight as to why it is not finding the terminal operation? Do you have ideas how to fix it? Let's discuss it prior to executing a particular strategy.
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.
If the test case is:
class A {
Stream<Object> m() {
Stream<Object> stream = new ArrayList<>().stream().sorted();
return stream;
}
@EntryPoint
void n() {
Stream<Object> s = m();
s.distinct().count();
}
}
the ordering is 'Ordered' and the project throws an exception (require terminal operations).
If you replace HashSet() with ArrayList, is it still UNORDERED? That would answer whether it is just accepting a default value.
According to the test result above, the answer should be yes.
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.
Do you have any insight as to why it is not finding the terminal operation? Do you have ideas how to fix it? Let's discuss it prior to executing a particular strategy.
I came up with a question in #103 . I found the instance of stream just processes the operations in the method of enclosingMethodDeclaration.
For the example below:
class A {
Stream<Object> m() {
Stream<Object> stream = new HashSet<>().parallelStream();
return stream;
}
@EntryPoint
void n() {
Stream<Object> s = m();
s.count();
}
}
Do we need to create an instance of stream for n()? Can we reuse the instance of stream for m()? Those are what I am thinking.
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.
No variation of execution modes in a single example.
|
||
public void testNonInternalAPI4() throws Exception { | ||
helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()", | ||
Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false, |
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.
If you replace HashSet()
with ArrayList
, is it still UNORDERED
? That would answer whether it is just accepting a default value.
|
||
public void testNonInternalAPI4() throws Exception { | ||
helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()", | ||
Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false, |
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.
Do you have any insight as to why it is not finding the terminal operation? Do you have ideas how to fix it? Let's discuss it prior to executing a particular strategy.
class A { | ||
|
||
Stream<Object> m() { | ||
Stream<Object> stream = new HashSet<>().parallelStream().sorted(); |
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.
I fail to see how this is varying the execution order. Particularly, I should see a call to either parallel()
or sequential()
rather than something like sorted()
.
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.
Please add one more test case similar to testNonInternalAPI4() but varying execution modes.
Is .parallelStream()
not enough?
sorted()
influences ordering and .parallelStream()
influences execution mode. You want to see different execution mode or ordering?
So it sounds like it's just using a default value for the ordering and not any of the typestate analysis.
On Fri, 2017-12-01 at 11:11 -0800, Grace Tang wrote:
@saledouble commented on this pull request.
________________________________
In edu.cuny.hunter.streamrefactoring.tests/test cases/edu/cuny/hunter/streamrefactoring/ui/tests/ConvertStreamToParallelRefactoringTest.java<#124 (comment)>:
+ helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()",
+ Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false,
+ false, null, null, null, RefactoringStatus.ERROR,
+ EnumSet.of(PreconditionFailure.NO_TERMINAL_OPERATIONS)));
+ }
+
+ public void testNonInternalAPI3() throws Exception {
+ helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()",
+ Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false,
+ false, null, null, null, RefactoringStatus.ERROR,
+ EnumSet.of(PreconditionFailure.NO_TERMINAL_OPERATIONS)));
+ }
+
+ public void testNonInternalAPI4() throws Exception {
+ helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()",
+ Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false,
If the test case is:
class A {
Stream<Object> m() {
Stream<Object> stream = new ArrayList<>().stream().sorted();
return stream;
}
@entrypoint
void n() {
Stream<Object> s = m();
s.distinct().count();
}
}
the ordering is 'Ordered' and the project throws an exception (require terminal operations).
[image]<https://user-images.githubusercontent.com/10117031/33499088-8966e744-d6a1-11e7-801c-c101a32ceb61.png>
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DPz4OSoWH847MyfFPPMytckF1voSJks5s8E9ygaJpZM4QxWc4>.
|
Please be specific. Do you mean an instance of edu.cuny.hunter.streamrefactoring.core.analysis.Stream?
On Fri, 2017-12-01 at 19:24 +0000, Grace Tang wrote:
@saledouble commented on this pull request.
________________________________
In edu.cuny.hunter.streamrefactoring.tests/test cases/edu/cuny/hunter/streamrefactoring/ui/tests/ConvertStreamToParallelRefactoringTest.java<#124 (comment)>:
+ helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()",
+ Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false,
+ false, null, null, null, RefactoringStatus.ERROR,
+ EnumSet.of(PreconditionFailure.NO_TERMINAL_OPERATIONS)));
+ }
+
+ public void testNonInternalAPI3() throws Exception {
+ helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()",
+ Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false,
+ false, null, null, null, RefactoringStatus.ERROR,
+ EnumSet.of(PreconditionFailure.NO_TERMINAL_OPERATIONS)));
+ }
+
+ public void testNonInternalAPI4() throws Exception {
+ helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()",
+ Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.UNORDERED), false, false,
Do you have any insight as to why it is not finding the terminal operation? Do you have ideas how to fix it? Let's discuss it prior to executing a particular strategy.
I came up with a question in #103<#103> . I found the instance of stream just processes the operations in the method of enclosingMethodDeclaration.
For the example below:
class A {
Stream<Object> m() {
Stream<Object> stream = new HashSet<>().parallelStream();
return stream;
}
@entrypoint
void n() {
Stream<Object> s = m();
s.count();
}
}
Do we need to create an instance of stream for n()? Can we reuse the instance of stream for m()? Those are what I am thinking.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP-D5tUf9yjISDtLrYKdhj6_adzmSks5s8FJ-gaJpZM4QxWc4>.
|
Yes.
I found there is only one instance of Stream. What this instance does is limited to one method m(). I did not find any instance of Stream to operate n(). |
The is only one instance of edu.cuny.hunter.streamrefactoring.core.analysis.Stream is that example. But, even if n() did not exist, there would still be only one, so I'm unsure how the question is relevant to this interprocedural case. Could you please explain?
On Fri, 2017-12-01 at 14:56 -0800, Grace Tang wrote:
Do you mean an instance of edu.cuny.hunter.streamrefactoring.core.analysis.Stream?
Yes.
For the example below:
class A {
Stream<Object> m() {
Stream<Object> stream = new ArrayList<>().stream().sorted();
return stream;
}
@entrypoint
void n() {
Stream<Object> s = m();
s.distinct().count();
}
}
I found there is only one instance of Stream. What this instance doing is limited to one method m(). I did not find any instance of Stream to operate n().
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP2qIy9Hw2_Txe6dzADkNDQdrgwVbks5s8IQvgaJpZM4QxWc4>.
|
Because there is only one instance of What I am doing: |
Just changing the value of |
What do you mean by changing the value of this method? It is not a variable.
Raffi Khatchadourian
Assistant Professor, Computer Science, Hunter College
Doctoral Faculty of the Graduate School and University Center's PhD Program in Computer Science
City University of New York
695 Park Avenue
Room HN 1090-H
New York, NY 10065
P: (212) 650-3988
F: (212) 772-5219
[email protected]
http://cuny.is/khatchad
On Dec 1, 2017 7:25 PM, Grace Tang <[email protected]> wrote:
Just changing the value of enclosingMethodDeclaration cannot work. Too many low level implementations are needed to change.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP1rAAJK5YMmr1PPbvldwRQ9TplvZks5s8JjwgaJpZM4QxWc4>.
|
Do you mean the method call StreamStateMachine(this).start();?
Raffi Khatchadourian
Assistant Professor, Computer Science, Hunter College
Doctoral Faculty of the Graduate School and University Center's PhD Program in Computer Science
City University of New York
695 Park Avenue
Room HN 1090-H
New York, NY 10065
P: (212) 650-3988
F: (212) 772-5219
[email protected]
http://cuny.is/khatchad
On Dec 1, 2017 6:21 PM, Grace Tang <[email protected]> wrote:
Because there is only one instance of edu.cuny.hunter.streamrefactoring.core.analysis.Stream, the method new StreamStateMachine(this).start(); in edu.cuny.hunter.streamrefactoring.core.analysis.Stream only processes one method in test case, i.e., only processes the method where the stream is created in the test case. For the test case above, it only processes m() ( stream is created in m).
What I am doing:
After initializing the stream, e.g. calling this.inferInitialExecution(); in edu.cuny.hunter.streamrefactoring.core.analysis.Stream, I check whether the current method(where the stream is created) in test case returns a stream (I finished this part). If no, I will not change anything. If yes, I want to change the value the enclosingMethodDeclaration before calling new StreamStateMachine(this).start();. I want the initialized stream with the new value of enclosingMethodDeclaration as a new input for StreamStateMachine(this).
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP_bjiN9G2FiUztQnyUXa93XK5BkEks5s8InfgaJpZM4QxWc4>.
|
I don't think that is entirely true because the typestate analysis uses the entire call graph AFAIK. As discussed in our status meeting, some things are intraprocedural while others are interprocedural.
Raffi Khatchadourian
Assistant Professor, Computer Science, Hunter College
Doctoral Faculty of the Graduate School and University Center's PhD Program in Computer Science
City University of New York
695 Park Avenue
Room HN 1090-H
New York, NY 10065
P: (212) 650-3988
F: (212) 772-5219
[email protected]
http://cuny.is/khatchad
On Dec 1, 2017 6:21 PM, Grace Tang <[email protected]> wrote:
Because there is only one instance of edu.cuny.hunter.streamrefactoring.core.analysis.Stream, the method new StreamStateMachine(this).start(); in edu.cuny.hunter.streamrefactoring.core.analysis.Stream only processes one method in test case, i.e., only processes the method where the stream is created in the test case. For the test case above, it only processes m() ( stream is created in m).
What I am doing:
After initializing the stream, e.g. calling this.inferInitialExecution(); in edu.cuny.hunter.streamrefactoring.core.analysis.Stream, I check whether the current method(where the stream is created) in test case returns a stream (I finished this part). If no, I will not change anything. If yes, I want to change the value the enclosingMethodDeclaration before calling new StreamStateMachine(this).start();. I want the initialized stream with the new value of enclosingMethodDeclaration as a new input for StreamStateMachine(this).
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP_bjiN9G2FiUztQnyUXa93XK5BkEks5s8InfgaJpZM4QxWc4>.
|
You're strategy will not work in part because it is too specific. This is a fundamental problem with test driven development. You are focused so much on the test case that your strategy will not account for the general case where streams can be passed as parameters and stored into fields.
Raffi Khatchadourian
Assistant Professor, Computer Science, Hunter College
Doctoral Faculty of the Graduate School and University Center's PhD Program in Computer Science
City University of New York
695 Park Avenue
Room HN 1090-H
New York, NY 10065
P: (212) 650-3988
F: (212) 772-5219
[email protected]
http://cuny.is/khatchad
On Dec 1, 2017 6:21 PM, Grace Tang <[email protected]> wrote:
Because there is only one instance of edu.cuny.hunter.streamrefactoring.core.analysis.Stream, the method new StreamStateMachine(this).start(); in edu.cuny.hunter.streamrefactoring.core.analysis.Stream only processes one method in test case, i.e., only processes the method where the stream is created in the test case. For the test case above, it only processes m() ( stream is created in m).
What I am doing:
After initializing the stream, e.g. calling this.inferInitialExecution(); in edu.cuny.hunter.streamrefactoring.core.analysis.Stream, I check whether the current method(where the stream is created) in test case returns a stream (I finished this part). If no, I will not change anything. If yes, I want to change the value the enclosingMethodDeclaration before calling new StreamStateMachine(this).start();. I want the initialized stream with the new value of enclosingMethodDeclaration as a new input for StreamStateMachine(this).
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP_bjiN9G2FiUztQnyUXa93XK5BkEks5s8InfgaJpZM4QxWc4>.
|
Altering the getEnclosingMethodDeclaration() is not necessary. It works as expected. In other words, it does exactly what the name of the method says it does.
The changes will most likely be entirely in the start() method of StreamStateMachine. We should change the parts of that method that just look at the enclosing method to instead consider other methods as well.
Raffi Khatchadourian
Assistant Professor, Computer Science, Hunter College
Doctoral Faculty of the Graduate School and University Center's PhD Program in Computer Science
City University of New York
695 Park Avenue
Room HN 1090-H
New York, NY 10065
P: (212) 650-3988
F: (212) 772-5219
[email protected]
http://cuny.is/khatchad
On Dec 1, 2017 7:25 PM, Grace Tang <[email protected]> wrote:
Just changing the value of enclosingMethodDeclaration cannot work. Too many low level implementations are needed to change.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP1rAAJK5YMmr1PPbvldwRQ9TplvZks5s8JjwgaJpZM4QxWc4>.
|
Changing call sites can work. I did not find a method to examine the call sites for all method, so I examine the call sites for the methods in the context of enclosing method and enclosing method itself (this may be smarter). The new pull request also needs a new pull request for WALA. |
I think that it is better to have the functionality and design correct and in agreement, respectively, prior to "being smarter." Once we are sure of the former two, we can think of ways to improve. |
@@ -636,6 +637,11 @@ private void inferInitialOrdering() | |||
*/ | |||
protected CGNode getEnclosingMethodNode() throws IOException, CoreException, NoEnclosingMethodNodeFoundException { | |||
MethodReference methodReference = this.getEnclosingMethodReference(); | |||
return getEnclosingMethodNode(methodReference); |
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.
Per this comment, I advised against altering getEnclosingMethodNode()
.
return getEnclosingMethodNode(methodReference); | ||
} | ||
|
||
protected CGNode getEnclosingMethodNode(MethodReference methodReference) |
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.
This parameter makes no sense and has not associated documentation.
@@ -644,6 +650,19 @@ protected CGNode getEnclosingMethodNode() throws IOException, CoreException, NoE | |||
return nodes.iterator().next(); // just return the first. | |||
} | |||
|
|||
protected HashSet<CGNode> getEnclosingMethodNodes() |
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.
Because portions of our call graph are context sensitive, there may be more than one node that represents a single method. But, they are basically the same and are only used methods returning streams. The only difference is the calling context. I don't see any documentation on why all methods representing the enclosing method are needed. Furthermore, there was no internal discussion of this strategy.
+ " that was originally: " + previousReceivers; | ||
|
||
++processedInstructions; | ||
// CGNode cgNode = this.getStream().getEnclosingMethodNode(); |
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.
I will not accept commented code.
|
||
++processedInstructions; | ||
// CGNode cgNode = this.getStream().getEnclosingMethodNode(); | ||
for (CGNode cgNode : this.getStream().getEnclosingMethodNodes()) { |
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.
Why are these diff blocks so large? I cannot see what has changed.
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.
I just move the block into a loop and change its format by shift + ctrl + f。
|
||
++processedInstructions; | ||
// CGNode cgNode = this.getStream().getEnclosingMethodNode(); | ||
for (CGNode cgNode : this.getStream().getEnclosingMethodNodes()) { |
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.
I am unsure of:
- What the collection of enclosing method nodes represents in this context, and
- why there are needed.
In other words, there is a lot of documentation missing here.
|
||
++processedInstructions; | ||
// CGNode cgNode = this.getStream().getEnclosingMethodNode(); | ||
for (CGNode cgNode : this.getStream().getEnclosingMethodNodes()) { |
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.
Why not, for starters, iterate over all nodes in the graph? If that can cover all cases (i.e., returning streams, passing streams, and storing streams into fields), and we can create proper unit tests for that functionality, we can then think about how to reduce the search space while still maintaining correctness.
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.
In the last weekend, I tried to iterate over all nodes, but got a new exception, so I tried the current strategy to see whether changing call sites can work. However, you are right. Although the current strategy may be smarter (it just iterates over the necessary node), it needs much more time to check whether the strategy are absolutely right. Hence, I will follow your advice.
OK.
On Tue, 2017-12-05 at 23:11 -0800, Grace Tang wrote:
@saledouble commented on this pull request.
________________________________
In edu.cuny.hunter.streamrefactoring.core/src/edu/cuny/hunter/streamrefactoring/core/analysis/StreamStateMachine.java<#124 (comment)>:
- // object instances that the receiver
- // reference points to.
- OrdinalSet<InstanceKey> pointsToSet = engine.getPointerAnalysis()
- .getPointsToSet(pointerKey);
- assert pointsToSet != null : "The points-to set (I think) should not be null for pointer: "
- + pointerKey;
-
- OrdinalSet<InstanceKey> previousReceivers = terminalBlockToPossibleReceivers
- .put(blockInContext, pointsToSet);
- assert previousReceivers == null : "Reassociating a blockInContext: "
- + blockInContext + " with a new points-to set: " + pointsToSet
- + " that was originally: " + previousReceivers;
-
- ++processedInstructions;
+ // CGNode cgNode = this.getStream().getEnclosingMethodNode();
+ for (CGNode cgNode : this.getStream().getEnclosingMethodNodes()) {
I just move the block into a loop and change its format by shift + ctrl + f。
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DPwH_nVAbU3SfmOfCxj29bL_zcAdJks5s9j4GgaJpZM4QxWc4>.
|
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.
Besides the comments I've entered, I am still not seeing test cases corresponding to passing Streams as parameters and methods communicating through a field (i.e., one method writes to a stream field and the other reads it).
* @throws CoreException | ||
* @throws NoEnclosingMethodNodeFoundException | ||
*/ | ||
protected HashSet<CGNode> getCGNodesInGraph() throws IOException, CoreException { |
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.
Why not just use the iterator?
cgNodes.add(cgNodeIterator.next()); | ||
} | ||
|
||
return cgNodes; |
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.
Call graphs can be massive. We don't want an extra step here of adding them to a set if it is not necessary.
+ " that was originally: " + previousReceivers; | ||
|
||
++processedInstructions; | ||
for (CGNode cgNode : this.getStream().getCGNodesInGraph()) { |
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.
You would use the iterator directly here.
I am also curious about the additional run time to examine each node in the call graph. I'm especially interested in tests that don't need it. I am wondering:
Please report on these. |
Do you mean :
|
|
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.
There is some strangeness in the test code. Please investigate.
|
||
helper(new StreamAnalysisExpectedResult("new HashSet<>().stream()", | ||
Collections.singleton(ExecutionMode.SEQUENTIAL), orderings, false, true, false, null, null, null, | ||
RefactoringStatus.ERROR, EnumSet.of(PreconditionFailure.INCONSISTENT_POSSIBLE_ORDERINGS))); |
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.
Why inconsistent orderings?
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.
Is that supposed to be the case? Please investigate.
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.
@saledouble Any progress on this?
public void testNonInternalAPI5() throws Exception { | ||
HashSet<ExecutionMode> executionModes = new HashSet<>(); | ||
executionModes .add(ExecutionMode.PARALLEL); | ||
executionModes .add(ExecutionMode.SEQUENTIAL); |
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.
@saledouble There's also a similar problem here. Do we have a bug?
This line is to add a new element into possibleExecutionModes. |
This line generates the wrong state: It generates bottom at first, then generates parallel. |
Interesting, thanks for the info. I think we have had a bug related to this recently but I thought it was fixed. Can you find it in the bug list? It might be closed.
On Fri, 2017-12-08 at 19:08 +0000, Grace Tang wrote:
I guess something wrong in instanceBlockStateTable
https://github.com/ponder-lab/Java-8-Stream-Refactoring/blob/25f026c52309d267cc21fce66bfffe99c45153c6/edu.cuny.hunter.streamrefactoring.core/src/edu/cuny/hunter/streamrefactoring/core/analysis/StreamStateMachine.java#L1141
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DPweaMIeOLAdqnOQ5Lm7Lt65fPhM3ks5s-YkRgaJpZM4QxWc4>.
|
It may be relevant to to #62 |
This sounds like a new problem. Please file a bug with a test case and related it to #103. Also, for the changes for #103, reference the new bug number in comments near the test case lines related to this bug. Thanks.
On Fri, 2017-12-08 at 18:52 +0000, Grace Tang wrote:
This line generates the wrong state:
https://github.com/ponder-lab/Java-8-Stream-Refactoring/blob/25f026c52309d267cc21fce66bfffe99c45153c6/edu.cuny.hunter.streamrefactoring.core/src/edu/cuny/hunter/streamrefactoring/core/analysis/StreamStateMachine.java#L1146
It generates bottom at first, then generates parallel.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#124 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DP2R7gDAkbQjkimxolqoVoWi1UrnVks5s-YVXgaJpZM4QxWc4>.
|
|
||
++processedInstructions; | ||
// CGNode cgNode = this.getStream().getEnclosingMethodNode(); | ||
for (CGNode cgNode : this.getStream().getEnclosingMethodNodes()) { |
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.
In the last weekend, I tried to iterate over all nodes, but got a new exception, so I tried the current strategy to see whether changing call sites can work. However, you are right. Although the current strategy may be smarter (it just iterates over the necessary node), it needs much more time to check whether the strategy are absolutely right. Hence, I will follow your advice.
@@ -281,114 +280,115 @@ public void start() throws IOException, CoreException, CallGraphBuilderCancelExc | |||
// the node of where the stream was declared: TODO: Can this be | |||
// somehow rewritten to get blocks corresponding to terminal | |||
// operations? | |||
CGNode cgNode = this.getStream().getEnclosingMethodNode(); | |||
|
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.
Just add a FOR
statement. It changes the format of the block in the loop, so red block is large.
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.
The loop is from line 283 to line 396.
throws IOException, CoreException, NoEnclosingMethodNodeFoundException { | ||
HashSet<CGNode> cgNodes = new HashSet<>(); | ||
|
||
Iterator<CGNode> cgNodeIterator = this.getAnalysisEngine().getCallGraph().iterator(); |
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.
Finally, I found a easy way to get CGNodes in the call graph. I tried a complex way in the weekend and got an exception.
Sorry, by comments, I meant comments in the *code*.
|
Fix for #103.