mbeckerle commented on code in PR #1423:
URL: https://github.com/apache/daffodil/pull/1423#discussion_r1949393221
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Coroutines.scala:
##########
@@ -212,13 +213,13 @@ final class InvertControl[S](body: => Unit) extends
MainCoroutine[Try[S]] with I
private val dummy: Try[S] = Success(null.asInstanceOf[S])
- private def gen: Stream[S] = {
+ private def gen: LazyList[S] = {
val x = resume(
producer,
dummy
) // producer isn't sent anything. It's just resumed to get another value.
x match {
- case EndOfData => Stream.Empty
+ case EndOfData => LazyList.empty
Review Comment:
Add comment `// TODO: no test coverage`
##########
daffodil-lib/src/main/scala/passera/numerics/package.scala:
##########
@@ -30,16 +30,16 @@ import scala.language.implicitConversions
package object numerics {
implicit def toRicherInt(x: Int): RicherInt = new RicherInt(x)
- implicit def toRicherInt(x: scala.runtime.RichInt) = new YetRicherInt(
+ implicit def toRicherInt(x: scala.runtime.RichInt): YetRicherInt = new
YetRicherInt(
Review Comment:
I would suggest don't bother putting any comments into the passera library
code about test coverage that is missing.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/StackLike.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 scala.collection.mutable.ListBuffer
+
+/**
+ * Compatibility class for 2.12 and 2.13 since ArrayStack and Stack
+ * have been deprecated in 2.13. This allows us to maintain the same
+ * functionality as stack while using ListBuffer instead
+ *
+ * ArrayDeque is the recommended replacement but is not available in 2.12
+ *
+ * TODO: If 2.12 support is dropped, this class should be deleted and
ArrayDeque used
+ * instead
+ */
+class StackLike[T] {
Review Comment:
StackLike sounds like a name of a trait, not a concrete class. Is there a
reason this can't just be Stack?
I would even suggest Stack1 is better than StackLike, because Stack1
suggests the name of a concrete instantiable class.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/CodeGeneratorState.scala:
##########
@@ -161,39 +170,41 @@ class CodeGeneratorState(private val root: ElementBase) {
def popArray(context: SchemaComponent): Unit = {
// Finish generating array element
val e = context.asInstanceOf[ElementBase]
- val C = structs.top.C
+ val C = structs.top().C
val arrayName = s"array_${cStructName(e)}$C"
// Prevent redundant definitions on reused types
if (elementNotSeenYet(e, arrayName)) {
addArrayImplementation(e)
}
// Link parent element to array element
- val declarations = structs.top.declarations
- val offsetComputations = structs.top.offsetComputations
- val erdComputations = structs.top.erdComputations
+ val declarations = structs.top().declarations
+ val offsetComputations = structs.top().offsetComputations
+ val erdComputations = structs.top().erdComputations
structs.pop()
- structs.top.declarations ++= declarations
- structs.top.offsetComputations ++= offsetComputations
- structs.top.erdComputations ++= erdComputations
+ structs.top().declarations ++= declarations
+ structs.top().offsetComputations ++= offsetComputations
+ structs.top().erdComputations ++= erdComputations
// Now call the array's methods instead of the array's element's methods
val indent = if (hasChoice) INDENT else NO_INDENT
if (hasChoice)
- structs.top.initChoiceStatements += s"$indent
${arrayName}_initERD(instance, parent);"
+ structs
+ .top()
+ .initChoiceStatements += s"$indent ${arrayName}_initERD(instance,
parent);"
Review Comment:
I suggest just inserting a comment at these statements indicating they are
not covered by tests
E.g.,
```
// TODO: not convered by tests
```
But don't bother to try to figure them out.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/StackLike.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 scala.collection.mutable.ListBuffer
+
+/**
+ * Compatibility class for 2.12 and 2.13 since ArrayStack and Stack
+ * have been deprecated in 2.13. This allows us to maintain the same
+ * functionality as stack while using ListBuffer instead
+ *
+ * ArrayDeque is the recommended replacement but is not available in 2.12
Review Comment:
Drop comment.
Even if ArrayDeque is recommended, there is value in calling a stack a
Stack. I would not recommend removing this even if ArrayDeque has all the
functionality, because well, an ArrayDeque presumably also has non-stack
additional behavior and if we are only using the Stack behavior then that is
worth knowing, preferably by having this class/trait that enforces this.
I mean if I am reading code, and it uses a data structure that uses "double
ended queue", then I'm assuming they chose that because they need more than
just stack behavior. So it's misleading to me if someone only needs stack
behavior, but uses a more general data structure which does not have 'stack' in
its name.
--
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]
