> On Nov 3, 2015, at 3:28 AM, Paul Sandoz <paul.san...@oracle.com> wrote:
>
> Hi Mandy,
>
> This is very nice work.
>
> Comments below, mostly minor stuff.
>
Thanks for the review.
I fixed most of the comments below. One question:
Is the name “StackStream" inappropriate and its subtypes? I prefer StackStream
to StackTraverser.
>
> PlatformLogger.java
> (and similar comments for duplicated code in LogRecord.java)
> —
>
> 542 static final StackWalker stackWalker;
>
> Use capitals for static variable.
>
Fixed.
>
> 556 private boolean lookingForLogger = true;
> 557 /**
>
> Missing line-feed.
>
Fixed.
>
> 588 private String getCallerInfo() {
> 589 Optional<StackWalker.StackFrame> frame = new
> CallerFinder().get();
> 590 if (frame.isPresent()) {
> 591 StackWalker.StackFrame f = frame.get();
> 592 return f.getClassName() + " " + f.getMethodName();
> 593 } else {
> 594 return name;
> 595 }
> 596 }
>
>
> Consider using:
>
> return frame.map(f -> f.getClassName() + " " +
> f.getMethodName()).orElse(name);
>
Thanks for the nicer alternative.
> You might want to highlight that CallerFinder is a stateful predicate.
>
>
> LogRecord.java
> —
>
> 642 // Skip all frames until we have found the first logger frame.
> 643 Optional<StackWalker.StackFrame> frame = new CallerFinder().get();
> 644 if (frame.isPresent()) {
> 645 StackWalker.StackFrame f = frame.get();
> 646 setSourceClassName(f.getClassName());
> 647 setSourceMethodName(f.getMethodName());
> 648 }
> 649 // We haven't found a suitable frame, so just punt. This is
> 650 // OK as we are only committed to making a "best effort" here.
> 651 }
>
> Consider using:
>
> frame.ifPresent(f -> {
> setSourceClassName(f.getClassName());
> setSourceMethodName(f.getMethodName());
> });
>
Fixed.
>
> LiveStackFrame
> —
>
> 41 /**
> 42 * Return the monitors held by this stack frames. This method returns
> 43 * an empty array if no monitor is held by this stack frame.
> 44 *
> 45 * @return the monitors held by this stack frames
> 46 */
> 47 public Object[] getMonitors();
>
> s/this stack frames/stack frame
>
> Can the contents of the array be modified?
>
>
Yes this can be modified. Each stack frame has its own arrays.
> 49 /**
> 50 * Gets the local variable array of this stack frame.
> 51 *
> 52 * <p>A single local variable can hold a value of type boolean, byte,
> char,
> 53 * short, int, float, reference or returnAddress. A pair of local
> varaibles
> 54 * can hold a value of type long or double. In other words,
> 55 * a value of type long or type double occupies two consecutive local
> 56 * variables. For a value of primitive type, the element in the
> 57 * local variable array is an {@link PrimitiveValue} object;
> 58 * otherwise, the element is an {@code Object}.
> 59 *
> 60 * <p>A value of type long or double will be represented by
> 61 * a {@code PrimitiveValue} object containing the value in the
> elements
> 62 * at index <em>n</em> and <em>n</em>+1 in the local variable array.
> 63 *
> 64 * @return the local variable array of this stack frame.
> 65 */
> 66 public Object[] getLocals();
>
> s/varaibles/variables
>
> (double slots are a PITA!)
>
Yes it is.
>
> StackStream
> —
>
> Suggest using something other than Stream in the name is this is really about
> Spliterator and traversal e.g. perhaps StackTraverser. Same for sub-types of
> StackStream.
>
> 494 @Override
> 495 public void forEachRemaining(Consumer<? super StackFrame> action) {
> 496 throw new UnsupportedOperationException("Specialized
> StackWalker");
> 497 }
> 498
> 499 @Override
> 500 public boolean tryAdvance(Consumer<? super StackFrame> action) {
> 501 throw new UnsupportedOperationException("Specialized
> StackWalker");
> 502 }
>
> Is this required?
>
CallerClassStream and StackTrace don’t support to use with Stream API for
performance.
>
> StackWalker
> —
>
> 46 * The {@linkplain Stream#spliterator() spliterator} of the stream
> performs
> 47 * the stack frame traversal in an {@linkplain Spliterator#ORDERED
> ordered} manner.
>
> It’s probably best not to mention Spliterator. Suggest
>
> the stream reports stack frame elements in order, from the top most frame
> that represents the execution point at
> which the stack was generated to the bottom most frame...
>
>
> 64 * <p> <em>Examples</em>
>
> Use an @apiNote
>
>
> 82 public final class StackWalker {
> 83 /**
> 84 * A {@code StackFrame} object represent a method invocation returned
> by
> 85 * {@link StackWalker}.
>
>
> s/object represent/object represents
>
>
> 87 * <p> {@code StackFrame} objects returned by {@link StackWalker} may
> not implement
> 88 * the {@link #getDeclaringClass()} method
> 89 * depending on the requested {@linkplain Option stack walking
> options}.
>
> Rather than "may not implement” suggest something like:
>
> The getDeclaring class method may be unsupported as determined by the stack
> walking options of a stack walker.
>
>
> 118 * @throws UnsupportedOperationException if this {@code
> StackFrame}
> 119 * does not implement this method
>
> if this StackFrame does not support… as determined by …
>
>
> 137 public String getFileName();
> 138 /**
>
> Missing line-feed.
>
>
> 151 /**
> 152 * Returns true if the method containing the execution point
> 153 * represented by this stack frame is a native method.
> 154 *
> 155 * @return {@code true} if the method containing the execution
> point
> 156 * represented by this stack frame is a native method.
> 157 */
> 158 public boolean isNativeMethod();
>
> Consistently use {@code true}
>
>
> 160 /**
> 161 * Gets a {@code StackTraceElement} for this stack frame.
> 162 *
> 163 * @apiNote This method does not seem to be needed as user could
> simply
> 164 * use StackFrame methods directly.
> 165 *
> 166 * @return {@code StackTraceElement} for this stack frame.
> 167 *
> 168 * */
> 169 public default StackTraceElement asStackTraceElement() {
>
> I guess that @apiNote is a TODO? Perhaps remove it if you think it is not
> needed, unless it is useful for compatibility reasons, it might be e.g.
> consider map(StackFrame::asStackTraceElement)
>
>
>
> 238 private final List<? extends WalkerOption> options;
>
> Do you require the bound here?
>
Sorry WalkerOption has been removed. I only updated the API but not webrev.00.
>
> 241 /**
> 242 * Gets a {@code StackWalker} instance.
> 243 *
> 244 * <p> This {@code StackWalker} obtains the method name and the class
> name
> 245 * with all {@linkplain StackWalker.Option#SHOW_HIDDEN_FRAMES hidden
> frames}
> 246 * skipped.
> 247 */
> 248 public StackWalker() {
>
> s/Gets/Creates
>
> Same for other constructor methods.
>
>
> 252 /**
> 253 * Gets a {@code StackWalker} instance with the given options
> specifying
> 254 * the stack frame information it can access. If no option is
> specified, this
> 255 * {@code StackWalker} obtains the method name and the class name
> 256 * with all {@linkplain StackWalker.Option#SHOW_HIDDEN_FRAMES hidden
> frames}
> 257 * skipped.
>
> If no options are given…
>
> It might be easier to state as if the options were {CLASS_REFERENCE,
> SHOW_REFLECT_FRAMES}, perhaps it depends if the default is someone under
> specified i.e. is it guaranteed that the option set will be the complement of
> the single set SHOW_HIDDEN_FRAMES?
In fact the default can’t be expressed with the current options. This is a
good point though and I have been considering adding SHOW_METHOD_INFO which
will be the default. I’ll update the API.
>
> I guess any unrecognized options are ignored?
>
Now it takes only Option enums.
>
> 324 /**
> 325 * Applies the given function to the stream of {@code StackFrame}s
> 326 * for the current thread, traversing from the top of the stack,
> 327 * which is the method calling this method.
> 328 *
> 329 * <p>To find the first 10 calling frames, first skipping those
> frames whose
> 330 * declaring class belong to package {@code com.foo}:
> 331 * <blockquote>
> 332 * <pre>{@code List<StackFrame> frames = new StackWalker().walk((s)
> ->}
>
> } -> {
>
> 333 * s.dropWhile(f -> f.getClassName().startsWith("com.foo."))
> 334 * .limit(10)
> 335 * .collect(Collectors.toList()));
> 336 * </pre></blockquote>
> 337 * @apiNote The {@code Stream<StackFrame>} object will be closed when
> 338 * this method returns. When a closed {@code Stream<StackFrame>}
> object
> 339 * is reused, {@code IllegalStateException} will be thrown.
>
> Yay, dropWhile :-)
>
> Use an @apiNote for the example, the original note is i think normative.
>
> Probably need to mention that parallel execution will is effectively disabled
> and stream pipeline execution will only occur on the current thread.
>
>
>
> 402 @CallerSensitive
> 403 public <T> T walk(Function<Stream<StackFrame>, T> function,
> 404 Function<Integer, Integer> batchSizeMapper) {
>
> Can you use IntUnaryOperator?
>
Remi and David suggest that too.
>
> 430 /**
> 431 * Gets the {@code Class} object of the caller invoking the method
> 432 * that calls this {@code getCallerClass} method.
> 433 *
> 434 * <p>This is equivalent to calling
> 435 * <blockquote>
> 436 * <pre>{@code walk((s) ->}
>
> } -> {
>
> 437 * s.map(StackFrame::getDeclaringClass)
> 438 * .skip(2)
> 439 * .findFirst()).orElse(Thread.currentThread().getClass());
> 440 * </pre></blockquote>
>
>
> 441 *
> 442 * <p>For example, {@code Util::getResourceBundle} loads a resource
> bundle on behalf
> 443 * of the caller. It calls this {@code getCallerClass} method to
> find the method
> 444 * calling {@code Util::getResourceBundle} and use the caller's class
> loader to
> 445 * load the resource bundle. The caller class in this example is the
> {@code Foo} class.
>
>
> use @apiNote
>
>
> BatchSizeMapper
> —
>
> As usual i always advice using testng and data providers, up to you.
I started this one with TestNG but this data provider requires the method to
return Object[][]. I automatically resorted to old-school test :)
>
> 105 return frameCount = stream
> 106 .map(f -> {
> 107 if (verbose) System.out.println(f);
> 108 return f;
> 109 })
>
> You can use peek instead.
>
>
> GetCallerClassTest
> —
>
> 105 class Test1 {
> 106 static void test() {
> 107 GetCallerClassTest.staticGetCallerClass(Test1.class);
> 108 GetCallerClassTest.reflectiveGetCallerClass(Test1.class);
> 109 }
> 110 }
> 111
> 112 class Test2 {
> 113 static void test() {
> 114 GetCallerClassTest.staticGetCallerClass(Test2.class);
> 115 GetCallerClassTest.reflectiveGetCallerClass(Test2.class);
> 116 }
> 117 }
>
>
> Duplicate tests?
>
Probably extra-caution and can be removed.
> Suggest renaming tests because on failure it might easier to understand what
> failed.
>
Right - I have to review the tests one more pass before posting the updated
webrev.
>
> HiddenFrames
> —
>
> 64 void walk() {
> 65 Arrays.stream(new int[]{0})
> 66 .forEach(i -> walker.walk(s -> {
> 67 s.forEach(this::checkFrame);
> 68 return null;
> 69 }));
>
> Can you use Stream.of(0).forEach instead.
>
Yes Stream.of is better.
>
> Ran out of time :-)
Thanks a lot.
Mandy
