NightOwl888 commented on code in PR #1018:
URL: https://github.com/apache/lucenenet/pull/1018#discussion_r1842253910
##########
src/Lucene.Net.Tests/Codecs/Lucene40/TestBitVector.cs:
##########
@@ -44,14 +44,15 @@ public virtual void TestConstructSize()
DoTestConstructOfSize(1000);
}
- private void DoTestConstructOfSize(int n)
+ // LUCENENET specific - made static
+ private static void DoTestConstructOfSize(int n)
{
BitVector bv = new BitVector(n);
Assert.AreEqual(n, bv.Length); // LUCENENET NOTE: Length is the
equivalent of size()
}
/// <summary>
- /// Test the get() and set() methods on BitVectors of various sizes.
+ /// Test the <see cref="BitVector.Get(int)"/> and <see
cref="BitVector.Set(int)"/> methods on BitVectors of various sizes.
Review Comment:
I was looking at `BitVector` to see whether it is sensible to make these
into `this[int] { get; set; }` instead of (or in addition to) `Get(int)` and
`Set(int)`. Being that there is a `GetAndSet()` method (much like the atomic
classes), I am thinking not.
But I discovered that inside of several of the methods of `BitVector`, we
are throwing `IndexOutOfRangeException`. `IndexOutOfRangeException` is a
special exception type in .NET that should only be thrown from the getter of an
indexer. In virtually all other cases, these should be converted to
`ArgumentOutOfRangeException`. Looks like there are a handful of other types
that are throwing it, as well. We should open a new issue to convert these to
`ArgumentOutOfRangeException` and double-check that there will be no adverse
effects from making the change. These were originally intentionally normalized
to `IndexOutOfRangeException` to make the error handling simpler, but since
then we have fixed the error handling to catch either
`IndexOutOfRangeException` or `ArgumentOutOfRangeException` and treat them the
same using the [`IsIndexOutOfBoundsException()` extension
method](https://github.com/apache/lucenenet/blob/92029e8987076ff7264c4d5c8ce28ea77af9bd26/src/Lucene.Net/Support/Exception
Handling/ExceptionExtensions.cs#L260-L266) (although, it may not be used
everywhere it is needed to make the change).
I also noticed when searching for `IndexOutOfRangeException` that
`UTF8toUTF16` can throw it, also. This is unusual to have to deal with, so we
should change this method to `TryUTF8toUTF16` so we can eliminate this
exception that is clearly exclusively meant for control flow when the UTF8
format is invalid. It is caught in several places to do a fallback, and we
should fix this. So, we will need a new issue for that, as well. Note that we
will most likely be converting the `byte[]` overload to use
`ReadOnlySpan<byte>` and getting rid of the `offset` and `length` parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
