Minor clean up of some completion code#11609
Conversation
| if (!slot.Used) | ||
| { | ||
| break; | ||
| } |
There was a problem hiding this comment.
It's definitely too subtle, but this check is actually meaningful. When the _items array is initialized, it is filled with Slot structs containing all default values. So, if the _items slot hasn't been set yet, Used will be false indicating that there's no reason to continue searching.
The Used property is definitely a clumsy way to do this. Consider re-adding this test but change it to slot.CompletionList is null with a comment. It looks like CompletionListCache.Add(...) guarantees that CompletionList will be non-null for _items slots that have been set, so that should be functionally equivalent.
There was a problem hiding this comment.
Thank you for saying its subtle, because I definitely missed it! Glad our forebearers were there with uncommented code to prevent us iterating up to 9 times. Will fix.
There should probably be another test too, as this would have caused a null ref which clearly isn't covered.
There was a problem hiding this comment.
Honestly, I think it was me who didn't comment it. 😄 #stillgrowing
There was a problem hiding this comment.
Okay, I was wrong about the null ref, so didn't bother writing a test. I could artificially introduce a problem (ie, call .AssumeNotNull() on a non-nullable property, but it seems silly when this loop is pretty quick even if left unattended. There is a test that already covers the completely uninitialized state of the cache, so we'll know if we accidentally introduce an infinite loop or something.
| [return: NotNullIfNotNull(nameof(razorCompletionList))] | ||
| [return: NotNullIfNotNull(nameof(delegatedCompletionList))] |
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListCache.cs
Outdated
Show resolved
Hide resolved
…on/CompletionListCache.cs
| // CompletionList is annotated as non-nullable, but we are allocating an array of 10 items for our cache, so initially | ||
| // those array entries will be default. By checking for null here, we detect if we're hitting an unused part of the array | ||
| // so stop looping. |
Starting to look at cohosting completion resolve and noticed some things that annoyed me :)