Skip to content

Conversation

@FHannes
Copy link

@FHannes FHannes commented Oct 21, 2016

This commit is a patch for an incorrect refactoring of a foreach loop over a List to a sum() call on a Stream.
When the loop variable's type is a primitive int rather than an Integer, the current implementation did not add a mapToInt() call to obtain a specialized IntStream object first.

Example:

List<Integer> values = ...
int sum = 0;
for(int i : values) {
  sum += i;
}

The code above would refactor to:
int sum = values.stream().sum();

The correct refactoring is:
int sum = values.stream().mapToInt(i -> i).sum();

This commit is a patch for an incorrect refactoring of a foreach loop over a List<Integer> to a sum() call on a Stream.
When the loop variable's type is a primative int rather than an Integer, the current implementation did not add a mapToInt() call to obtain a specialized IntStream object first.

Example:
--------

List<Integer> values = ...
int sum = 0;
for(int i : values) {
  sum += i;
}

The code above would refactor to:
int sum = values.stream().sum();

The correct refactoring is:
int sum = values.stream().mapToInt(i -> i).sum();
@klikh
Copy link
Member

klikh commented Oct 22, 2016

Have you submitted a contributor license agreement? If not, please follow the steps described at http://www.jetbrains.com/agreements/cla/ to sign it.

@FHannes
Copy link
Author

FHannes commented Oct 22, 2016

I have signed the agreement.

@klikh
Copy link
Member

klikh commented Oct 22, 2016

Received, thank you.

SergeyZh pushed a commit that referenced this pull request Oct 24, 2016
…h primitive parameter (inspired by PR #455 by FHannes)
@amaembo
Copy link
Contributor

amaembo commented Oct 24, 2016

Thank you for discovering this bug and providing a pull-request. Indeed this case was not covered. Unfortunately the way you fixed this is not correct. First, it breaks primitive arrays iteration. For example:

  public int sum(int[] array) {
    int sum = 0;
    for(int x : array) {
      sum += x;
    }
    return sum;
  }

This case should be converted to Arrays.stream(array).sum(), but after applying your patch, it adds mapToInt(x -> x) step which is incorrect. Also it does not cover more complex problems. For example:

public class Main {
  public boolean check(Integer x) {
    return x % 3 == 0;
  }

  public boolean check(int x) {
    return x % 2 == 0;
  }

  public int sum(List<Integer> list) {
    int sum = 0;
    for(int x : list) {
      if(check(x))
        sum += x;
    }
    return sum;
  }

  public static void main(String[] args) {
    System.out.println(new Main().sum(Arrays.asList(1,2,3,4,5,6)));
  }
}

Here check(int) is called. After applying your fix we have compilable code, but it adds mapToInt after filter which silently changes the code semantics (calls check(Integer) now). What we should do is to modify the CollectionStream code generation adding mapToInt there. I decided that it would be easier to decline your pull request and fix this by myself in a proper way (see d51f09d). Nevertheless I really appreciate that you've found a bug in my code. Thank you and looking forward for new pull requests from you!

@amaembo amaembo closed this Oct 24, 2016
@FHannes
Copy link
Author

FHannes commented Oct 24, 2016

Alright, that's fine, I'm still in the process of learning how everything in the stream migration system operates. Thank you for the detailed overview!

SergeyZh pushed a commit that referenced this pull request Oct 26, 2016
…h primitive parameter (inspired by PR #455 by FHannes)

(cherry picked from commit d51f09d; IDEA-CR-14902)
intellij-monorepo-bot pushed a commit that referenced this pull request Nov 29, 2024
We weren't using the TextLinkStyles provided by the LinkAnnotation API,
and as a result, our links weren't stateful. We were also not properly
setting a disabled colour — now we do.

Also changed, we force the Markdown text to not be focusable, even if it
is clickable, since we don't want it to get focused. Now only links are
focused while tabbing through Markdown.

This also removes some testing harness left around from #425, and that
we don't need anymore.

Note: links should have a border around them when they are focused, but
that's not possible with the Compose APIs. What we do instead is show
a subtle background color,
taken from the ActionButtons' hover and pressed states, for our focused
and pressed states, respectively.
GitOrigin-RevId: 8cd3eee5791dbdb5f4f96f4e569e1f28923d1619
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.

3 participants