Hello Andreas,
thanks for taking care of Truffle API. Many of your changes are OK, here are few
comments about those that could be improved:
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/a360c82ba357
> GraphPrintVisitor implementation can now be disabled via system property
Such property is an API. Introducing new API into Truffle requires a review before
integration. Such review would reveal missing aspects of such change:
#1 it is not documented in the Javadoc, #2 it is not covered by a test. As a post-
mortem, please fix at least #1.
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/c1b1c06b49a3
> description:
> change SourceSection.toString()
Changes to various strings produced by objects in the Truffle API are API changes as
well. We cannot "just do them", as that is (functionally) backward incompatible
change. The previous value of toString() was useful, the new value is probably useful
as well, but I miss explanation why you did it? If that is a good reason, we need to stiff
(Truffle API amoeba shape)[http://wiki.apidesign.org/wiki/Amoeba] with a test, if it is
not, we need to modify the toString() behavior and stiff it in its new form. Personally I
found it dangerous to return programatically "useful" values from toString() methods
- people start relying on them instead of calling the right method - e.g. getCode() or
getShortDescription() - thus I would prefer harder to parse
return "SourceSection[name=" + getShortDescription + "]";
under the assumption that the only reason why you did the change was to simplify
debugging...
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/c9ba649b0dc1
> description:
> simplify LocationImpl.toString()
This is similar case to the previous change, but (assuming the previous return value
was cryptic enough to make no programmatic sense) changing debug only output is
probably OK.
Btw. I know the com.oracle.truffle.object.api package needs cleanup and polishing
and thanks for doing that, but I hope we'll be able to enter more stable mode by end
of the year 2015.
-jt
### SSW Notification Service : 13. 11. 2015 @ 16:50 ###
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/5d62f7cfe0e1
> changeset: 22362:5d62f7cfe0e1
> user: Andreas Woess <andreas.woess at oracle.com>
> date: Wed Nov 11 14:47:40 2015 +0100
> description:
> RootNode.reportLoopCount should never be part of compilation
>
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/a360c82ba357
> changeset: 22363:a360c82ba357
> user: Andreas Woess <andreas.woess at oracle.com>
> date: Fri Nov 13 11:29:47 2015 +0100
> description:
> GraphPrintVisitor implementation can now be disabled via system property
>
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/c1b1c06b49a3
> changeset: 22364:c1b1c06b49a3
> user: Andreas Woess <andreas.woess at oracle.com>
> date: Fri Nov 13 12:19:35 2015 +0100
> description:
> change SourceSection.toString()
>
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/c9ba649b0dc1
> changeset: 22365:c9ba649b0dc1
> user: Andreas Woess <andreas.woess at oracle.com>
> date: Fri Nov 13 14:41:11 2015 +0100
> description:
> simplify LocationImpl.toString()
>
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/78306843f20c
> changeset: 22366:78306843f20c
> user: Andreas Woess <andreas.woess at oracle.com>
> date: Fri Nov 13 15:36:25 2015 +0100
> description:
> minor Shape refactoring
>
> details: https://lafo.ssw.uni-linz.ac.at/hg/truffle/rev/1b48778cee21
> changeset: 22367:1b48778cee21
> user: Andreas Woess <andreas.woess at oracle.com>
> date: Fri Nov 13 16:25:04 2015 +0100
> description:
> add toString() methods to transitions
>
> diffstat:
>
> truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/nodes/GraphPrintV
> isitor.java | 73 ++++++++-
> truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/nodes/RootNode.ja
> va | 2 +
> truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/source/SourceSect
> ion.java | 2 +-
> truffle/com.oracle.truffle.object.basic/src/com/oracle/truffle/object/basic
> /ShapeBasic.java | 6 -
> truffle/com.oracle.truffle.object/src/com/oracle/truffle/object/LocationImp
> l.java | 6 +-
> truffle/com.oracle.truffle.object/src/com/oracle/truffle/object/ShapeImpl.j
> ava | 50 +-----
> truffle/com.oracle.truffle.object/src/com/oracle/truffle/object/Transition.
> java | 23 +++-
> truffle/com.oracle.truffle.object/src/com/oracle/truffle/object/debug/IGVSh
> apeVisitor.java | 2 +- 8 files changed, 99 insertions(+), 65 deletions(-)
>
> diffs (truncated from 387 to 50 lines):
>
> diff -r 3ed94f641d52 -r 1b48778cee21
> truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/nodes/GraphPrintV
> isitor.java ---
> a/truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/nodes/GraphPrin
> tVisitor.java Thu Nov 12 11:50:42 2015 -0800 +++
> b/truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/nodes/GraphPrin
> tVisitor.java Fri Nov 13 16:25:04 2015 +0100 @@ -59,6 +59,7 @@
>
> public static final String GraphVisualizerAddress = "127.0.0.1";
> public static final int GraphVisualizerPort = 4444;
> + private static final boolean ENABLED =
> !System.getProperty("truffle.graphprint", "").equalsIgnoreCase("false");
> private static final String DEFAULT_GRAPH_NAME = "truffle tree";
>
> private Map<Object, NodeElement> nodeMap;
> @@ -120,11 +121,29 @@
> }
> }
>
> - private static class Impl {
> + private interface Impl {
> + void writeStartDocument();
> +
> + void writeEndDocument();
> +