bsloane1650 commented on a change in pull request #207: Added support for
enumerations and TypeValueCalc
URL: https://github.com/apache/incubator-daffodil/pull/207#discussion_r275530412
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Cursor.scala
##########
@@ -93,9 +93,23 @@ trait Cursor[AccessorType <: Accessor[AccessorType]] {
* The inspectAccessor provides access to the data, the returned
* boolean indicates whether that happened successfully or there
* was no more data.
+ *
+ * Note that inspecting may have side effects. For instance, the
InfosetInputter
+ * returns a DINode on inspection. However, in order to construct a DINode,
said node
+ * must be inserted into the infoset. Therefore, in a chain with one or more
calls to inspect,
+ * followed by a call to advance, the side effect of constructing the DINode
will be realized on the
+ * first call to inspect.
+ *
+ * For a side-effect free variant, consider inspectPure
*/
def inspect: Boolean
+ /**
+ * Like inspect, but only partially populates the accessor with data that
can be computed without
Review comment:
To be honest, I am not particularly happy with anything about Cursor. Most
of my changes feel like a very hacky way to work through a leaky abstraction
without any significant refactoring (which I am not comfortable enough with
this area of the codebase to do confidently).
The core issue here is that the Cursor trait in daffodil-lib/.../util/ is
really an abstract type that is completely independent from any notion of
"infoset". Indeed, the daffodil-lib project is not even aware of what an
Infoset is, as it does not depend on the relevent project (and can't without
introducing a circular dependency). As it turns out, the only user of Curosr is
InfosetCursor.
The motiviation for "inspectPure" is that the implementation of "inspect" by
InfosetCursor is broken with regards to the standard meaning of "inspect"
(which is normally thought of as a side-effect free method), and it is
impossible to write a non-broken implementation in a backwards compatible way.
As it turns out, inspectPure isn't actually side effect free either (as it
consumes input from the underlying Inputter), but it should be free of
externally visible side effects.
Ultimately, the issue is that we are defining a really wonky API here.
Understanding the implications of inspect vs inspectPure really does require
looking at the actual implementing class to what what what side-effects inspect
has.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
