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

use QHash instead of QList for Document::sDocumentInstances #2159

Merged
merged 2 commits into from Jul 17, 2019

Conversation

@oncer
Copy link
Contributor

oncer commented Jul 16, 2019

See issue #2156

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 16, 2019

Thanks for this optimization!

I've pushed some improvements based on #2156 (comment), though I did not have time to test it. Can you help with testing the change?

This avoids recalculating the canonical path in the destructor and in
setFileName. It also makes the canonical path readily available when
searching the open documents for a particular file.
@bjorn bjorn force-pushed the oncer:hash-documentinstances branch from 834098d to 6288714 Jul 16, 2019
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 16, 2019

At least I could have made sure it compiles... :-)

@oncer

This comment has been minimized.

Copy link
Contributor Author

oncer commented Jul 16, 2019

It compiles for me, and works well for my use-case. I can confirm there is another performance boost when using the cached canonical path. Thank you for the fix!

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 17, 2019

Yeah, it didn't compile when I first pushed my commit, but I force-pushed a working change after the CI system told me about the problem. :-)

You're welcome and thanks again for bringing this up!

@bjorn bjorn merged commit b0c6ac3 into bjorn:master Jul 17, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.