Hi Mandy,
This is very nice work.
Comments below, mostly minor stuff.
PlatformLogger.java
(and similar comments for duplicated code in LogRecord.java)
—
542 static final StackWalker stackWalker;
Use capitals for static variable.
556 private boolean lookingForLogger = true;
557 /**
Missing line-feed.
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);
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());
});
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?
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!)
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?
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?
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?
I guess any unrecognized options are ignored?
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?
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.
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?
Suggest renaming tests because on failure it might easier to understand what
failed.
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.
Ran out of time :-)
Paul.
> On 30 Oct 2015, at 20:04, Mandy Chung <[email protected]> wrote:
>
> JEP 259: http://openjdk.java.net/jeps/259
>
> Javadoc for the proposed StackWalker API:
>
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>
> A simple way to walk the stack:
>
> StackWalker walker = new StackWalker(StackWalker.Option.CLASS_REFERENCE);
> walker.walk((s) -> s.filter(f ->
> interestingClasses.contains(f.getDeclaringClass())).findFirst());
>
> The current usage of sun.reflect.Reflection.getCallerClass(int depth) can be
> replaced with this StackWalker API.
>
> Any feedback on the proposed API is appreciated.
>
> Mandy
>
> P.S. webrev of the current implementation:
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
>
>
>
>
>
>
>
