-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Move a few types related to pattern matching to be structs instead of classes. #68101
Conversation
@@ -7,7 +7,7 @@ | |||
|
|||
namespace Microsoft.CodeAnalysis.FindSymbols | |||
{ | |||
internal class SearchQuery : IDisposable | |||
internal readonly struct SearchQuery : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was very close to being able to make the whole host of these into ref-struct types. But SearchQuery itself is captured into the heap through teh use of a "predicate" delegate it hands out to the compiler to do some work. So for now these have to be normal structs unfortunately.
|
||
return array; | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
if (_sourceLowerCaseCharacters == null) | ||
throw new ObjectDisposedException(nameof(EditDistance)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 I don't think this is working the way one would expect. This would only throw for the case of default(EditDistance)
, so InvalidOperationException
might be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct. will fix.
using System.Diagnostics; | ||
|
||
namespace Roslyn.Utilities | ||
{ | ||
internal class WordSimilarityChecker | ||
internal struct WordSimilarityChecker : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Likely want either [NonCopyable]
or readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. needs to be NonCopyable.
No description provided.