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

Avoid needless concatenations of warning/error messages #3465

Merged
merged 8 commits into from
May 24, 2022

Conversation

JaroslavTulach
Copy link
Member

Pull Request Description

While investigating #182234656 I've noticed that GatherDiagnostics calls distinctBy and uses message string. However such string has to be composed and that takes time:

image

This PR eliminates the need for concatenation by associating each Diagnostic with an array of keys used to compute equals and hashCode.

Important Notes

Checklist

Please include the following checklist in your PR:

  • [ x ] All code conforms to the Scala,
  • All code has been tested:
    • [ x ] Unit tests have been written where possible.
sbt:runtime> testOnly *GatherDiagnosticsTest*
[info] compiling 3 Scala sources to /home/devel/NetBeansProjects/enso/enso.master/engine/runtime/target/scala-2.13/classes ...
[info] GatherDiagnosticsTest:
[info] Error Gathering
[info] - should work with expression flow (424 milliseconds)
[info] - should work with module flow (24 milliseconds)
[info] - should avoid duplication (1 second, 78 milliseconds)
[info] Run completed in 2 seconds, 731 milliseconds.
[info] Total number of tests run: 3
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 3, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner May 21, 2022 06:20
@JaroslavTulach JaroslavTulach changed the title Wip/jtulach/avoid to string 182234656 Avoid needless concatenations of warning/error messages May 22, 2022
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks semantically dangerous if left as is.

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's awesome :)

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, much easier to parse equals

@JaroslavTulach JaroslavTulach self-assigned this May 24, 2022
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label May 24, 2022
@mwu-tow mwu-tow merged commit 2973cd2 into develop May 24, 2022
@mwu-tow mwu-tow deleted the wip/jtulach/AvoidToString_182234656 branch May 24, 2022 05:16
@wdanilo wdanilo mentioned this pull request Feb 6, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants