Hi Mandy,
Thanks for the feedback!
New webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8145686/webrev.01/
On 18/12/15 03:22, Mandy Chung wrote:
It’d be helpful to add some comment in the skipLoggingFrame method.
Maybe even better to change this method to isFilteredFrame??
Done. I renamed the method.
Reflection frames and lambda forms should already be skipped by StackWalker.
Good point.
The remaining to skip are method handles, doPrivileged, other logging
internal (do you have the case that the logging internal are called via
reflection and is that why they are included here)?
No - it's just that if you invoke Logger.log() through reflection
you don't want to see Method.invoke as the caller.
But since this should be skipped by the StackWalker anyway,
I removed it.
Looks like it’s good to check if this list should be cleaned up.
Done.
Should Formatting::skipLoggingFrame to return true if
System.Logger.class.isAssignableFrom(t.getDeclaringClass())
so that that check doesn’t need to be duplicated in both SimpleConsoleLogger
and LogRecord?
Done.
422 if (cname.startsWith("java.lang.System$Logger"))
return true;
This line can be removed.
Done.
>For instance, RMI has a logger that wraps a j.u.l.Logger so
>ideally that should be skipped too...
>JMX remote has a ClassLogger that should also be skipped.
>etc…
That might be the reason why skipLoggingFrame filters sun.rmi.runtime.Log? Is
this necessary?
Yes - that's the reason. At some point I'm planing to convert
components that can be converted to using System.getLogger.
RMI may be a tough nut to crack because IIRW it's calling
java.util.logging configuration methods.
This white list needs to be maintained manually which is fragile.
Hence the idea of skipping methods on classes that
implement System.Logger. Those won't need to be put in the whitelist.
And as a consequence they won't need to resort to calling logp in
order to work around the caller inference issue (which is good
since #1. logp may not be implemented by the backend, and #2. it
requires either using java.util.logging or using jdk internal APIs).
186 @Override public StackWalker run() {
- Nit: separate @Override into a separate line
Done.
Mandy
