stevedlawrence commented on code in PR #1427:
URL: https://github.com/apache/daffodil/pull/1427#discussion_r1951510857
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetImpl.scala:
##########
@@ -126,18 +126,15 @@ sealed trait DINode {
def isDefaulted: Boolean
def isHidden: Boolean
- def children: LazyList[DINode]
+ def indexOf(item: DINode): Int
+ def findByCondition(func: DINode => Boolean): Option[DINode]
Review Comment:
Thoughts on naming this `def find(...)`, that matches the `find` function in
standard scala collections. Or maybe since `findChild` which kindof matches
`child` and `numChildren`?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetImpl.scala:
##########
@@ -1319,12 +1316,16 @@ sealed class DISimple(override val erd:
ElementRuntimeData)
def primType: NodeInfo.PrimType = erd.optPrimType.orNull
- def contents: IndexedSeq[DINode] = IndexedSeq.empty
-
private var _stringRep: String = null
private var _bdRep: JBigDecimal = null
- override def children: LazyList[DINode] = LazyList.empty
+ override def child(index: Int): DINode = null
Review Comment:
Does this want to be `Assert.invariant` or something? Nothing is probably
going to check for null so it's just going to propogate elsewhere and lead to a
bug. The other functions are probably fine since they return values that will
usually be checked.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetImpl.scala:
##########
@@ -1221,20 +1217,22 @@ final class DIArray(
def namedQName = erd.namedQName
- protected final var _contents = new ArrayBuffer[DIElement](initialSize)
+ protected final val _contents = new ArrayBuffer1[DIElement](initialSize)
- override def children: LazyList[DINode] =
- _contents.view.map(_.asInstanceOf[DINode]).to(LazyList)
+ override def indexOf(item: DINode): Int =
_contents.indexOf(item.asInstanceOf[DIElement])
/**
* Used to shorten array when backtracking out of having appended elements.
*/
def reduceToSize(n: Int): Unit = {
- _contents = _contents.slice(0, n)
+ _contents.reduceToSize1(n)
}
- override def contents: IndexedSeq[DINode] = _contents
- def elementContents: IndexedSeq[DIElement] = _contents
+ override def numChildren: Int = _contents.length
+
+ override def child(index: Int): DINode =
_contents(index).asInstanceOf[DINode]
Review Comment:
Same here, can asInstanceOf be removed?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetImpl.scala:
##########
@@ -1221,20 +1217,22 @@ final class DIArray(
def namedQName = erd.namedQName
- protected final var _contents = new ArrayBuffer[DIElement](initialSize)
+ protected final val _contents = new ArrayBuffer1[DIElement](initialSize)
- override def children: LazyList[DINode] =
- _contents.view.map(_.asInstanceOf[DINode]).to(LazyList)
+ override def indexOf(item: DINode): Int =
_contents.indexOf(item.asInstanceOf[DIElement])
Review Comment:
Is asInstanceOf needed? `_contents(index)` is a `DIElement` so it's already
a `DINode`.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/ArrayBuffer1.scala:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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 org.apache.daffodil.lib.util
+
+import java.util
+import scala.collection.mutable.ArrayBuffer
+
+/**
+ * Compatibility ArrayBuffer class for 2.12 and 2.13 since reduceToSize
+ * has been removed in 2.13. This allows us to maintain the same
+ * functionality as 2.12 while upgraded to 2.13
+ */
+class ArrayBuffer1[T](initialSize: Int = 1) extends ArrayBuffer[T] {
+
+ def this() = this(1) // Default size if none is provided
+
+ // Preallocate space to avoid frequent resizing
+ this.sizeHint(initialSize)
+
+ def reduceToSize1(sz: Int): Unit = {
+ if (sz >= 0 && sz <= size0) {
+ // to ensure things are not left un-garbage collected
+ util.Arrays.fill(array, sz, size0, null)
+ size0 = sz
+ } else
+ throw new IllegalArgumentException(
+ "Invalid size: Must be between 0 and the current size of the buffer."
+ )
+ }
+}
+
+object ArrayBuffer1 extends ArrayBuffer {
+ def apply[T](): ArrayBuffer1[T] = {
+ new ArrayBuffer1[T]()
+ }
+
+ def apply[A](initialSize: Int): ArrayBuffer1[A] = {
+ new ArrayBuffer1[A](initialSize)
Review Comment:
The normal object ArrayBuffer apply method creates an ArrayBuffer with those
elements as the entries. It's not the size. For example, `ArrayBuffer(10)`
creates an ArrayBuffer with a single element with value `10`. It also supports
things like `ArrayBuffer(1, 2, 3, 4, ...)` for multiple elements. I'm not sure
we have to implement this, but if we do it should proably have the same
behavior as ArrayBuffer.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/ArrayBuffer1.scala:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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 org.apache.daffodil.lib.util
+
+import java.util
+import scala.collection.mutable.ArrayBuffer
+
+/**
+ * Compatibility ArrayBuffer class for 2.12 and 2.13 since reduceToSize
+ * has been removed in 2.13. This allows us to maintain the same
+ * functionality as 2.12 while upgraded to 2.13
+ */
+class ArrayBuffer1[T](initialSize: Int = 1) extends ArrayBuffer[T] {
+
+ def this() = this(1) // Default size if none is provided
Review Comment:
I think this constructor isn't needed since `initialSize` has a default
value.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/ArrayBuffer1.scala:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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 org.apache.daffodil.lib.util
+
+import java.util
+import scala.collection.mutable.ArrayBuffer
+
+/**
+ * Compatibility ArrayBuffer class for 2.12 and 2.13 since reduceToSize
+ * has been removed in 2.13. This allows us to maintain the same
+ * functionality as 2.12 while upgraded to 2.13
+ */
+class ArrayBuffer1[T](initialSize: Int = 1) extends ArrayBuffer[T] {
Review Comment:
Suggest we default to 16, that's what ArrayBuffer normally defaults
to--gives enough space to avoid allocations for small arrays but isn't too big.
--
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]
