Avoid allocations in LightupHelpers.CanWrap... methods #8107
Conversation
|
The CanWrap... methods don't allocate anymore (only one shown here): Before: After: |
martin-strecker-sonarsource
changed the title
| // Avoid creating the delegate if the value already exists | ||
| bool canCast; | ||
| if (!wrappedObject.TryGetValue(obj.GetType(), out canCast)) | ||
| if (!SupportedObjectWrappers.TryGetValue(underlyingType, out var wrappedObject)) |
There was a problem hiding this comment.
is there a reason not to make these changes upstream, and afterwards copy them here?
The initial idea of the ShimLayer was that we only keep the code from them. Modifying it may require changes in licensing, keeping track of the changes and mentioning them.
There was a problem hiding this comment.
actually the MIT license permits modification
still, I find it cleaner to make the change upstream first, improving your impact
It's true that this change will improve our analyzer first, we can merge it, but I'm afraid that if we don't keep track of what we modify, we'll get out of sync and in the future when doing updates we will forget that we did this and we'll just update the file from upstream , losing the change, because that's what we've done until now
There was a problem hiding this comment.
We are quite a lot out of sync with the two SourceGenrators already. I don't know about the other files, as it is unclear what we added and what was copied from StyleCop. This file is out of sync as well (we have 877 lines and StyleCop 678).
Waiting for the upstream repo to react is also an unfortunate burden. I can create a PR, but I would keep it separate from this one. Once the upstream PR is merged, we may copy the file content over, but it seems we added some functionality in this file in the past.
There was a problem hiding this comment.
yes, we should not wait for upstream but we should always contribute back, and it doesn't seem we have this process in place
There was a problem hiding this comment.
|
The upstream pull request was approved with changes: I synced this PR with the changes from there in c59a99b |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
I suggest making the PR title (which will be in the commit) like DotNetAnalyzers/StyleCopAnalyzers#3711 ? |
martin-strecker-sonarsource
changed the title
| { | ||
| canCast = wrappedObject.GetOrAdd( | ||
| obj.GetType(), | ||
| kind => underlyingType.GetTypeInfo().IsAssignableFrom(obj.GetType().GetTypeInfo())); |
There was a problem hiding this comment.
this was very weird... the key kind was not used at all...
|
pls don't forget to merge upstream as well DotNetAnalyzers/StyleCopAnalyzers#3711 (review) |
Fixes #8106