belliottsmith commented on code in PR #50: URL: https://github.com/apache/cassandra-accord/pull/50#discussion_r1242742975
########## accord-core/src/main/java/accord/utils/SimpleBitSet.java: ########## @@ -0,0 +1,348 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package accord.utils; + +import java.util.Arrays; + +import static java.lang.Long.highestOneBit; +import static java.lang.Long.lowestOneBit; +import static java.lang.Long.numberOfTrailingZeros; + +public class SimpleBitSet Review Comment: > Why is that? You can always extend BitSet and block mutable APIs and delegate to read-only apis... Similar in concept to how java.util.Collections#unmodifiableList works (proxy reads, block writes) Because `valueOf` cannot be used to construct a subclass of `BitSet`. If the constructor for `BitSet` were public or protected, I would have used it. > so you can recreate prevSetBitNotBefore inefficiently > where I pointed out that the development of a new data structure I have responded there already. The original data structure did not have this bug, and we were already using this data structure for the purpose. The bug was introduced later, which is a risk in software development. There are action items for expanding test coverage, and likely this would be detected by simulation even if this did not occur. These classes are quite simple, and likely to be thoroughly tested before the project is done. There is always a question of balancing time investment and outcome, and we are currently pushing for feature completeness, expecting a final pass to clean up minor issues. I therefore do not think it is particularly helpful to point to these kinds of issues as somehow emblematic of anything, when they occur as (or more) often in the *much* more complex code interactions we are layering on top of this, whether we use a standard library or otherwise. This kind of issue is very shallow, and was highly likely to be caught even if we did not write anymore tests. I can also point to historic cases where bugs were _included_ in the released software because we _removed_ simple custom data structures by the same argument you make, only to discover the standard library behaviour was surprising and lead to memory leaks. So, this is far from the slam dunk argument people often think it is. Wasting significant effort debating this kind of ideological point is also highly counterproductive, IMO. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
