Formatting exception message#16
Formatting exception message#16Chaarlesss wants to merge 3 commits intoMojang:masterfrom Chaarlesss:master
Conversation
| final Class<?> clazz = defineClass(transformedName, transformedClass, 0, transformedClass.length, codeSource); | ||
| cachedClasses.put(transformedName, clazz); | ||
| return clazz; | ||
| if (transformedClass != null) { |
There was a problem hiding this comment.
Honestly I don't think there is anything wrong with skipping the null check here. If it is null then it throws an exception which is caught and logged below. Thus adding more detail to the stack trace. More details, the better. So I don't see a need for this protection.
There was a problem hiding this comment.
IMO it is worth checking it, as otherwise it will NPE when trying to save the transformed class (DEBUG_SAVE == true), and when not debug saving, the exception it does print points to the classloader itself NPEing, not that it couldn't find the class source.
There's also zero point going through the transformer list with null bytes, unless you're expecting one of them might provide the data?!
| if (DEBUG) { | ||
| LogWrapper.log(Level.TRACE, e, "Exception encountered attempting classloading of %s", name); | ||
| LogManager.getLogger("LaunchWrapper").log(Level.ERROR, "Exception encountered attempting classloading of %s", e); | ||
| LogManager.getLogger("LaunchWrapper").log(Level.ERROR, String.format("Exception encountered attempting classloading of %s", name), e); |
There was a problem hiding this comment.
This however, is fine, cuz derp.
There was a problem hiding this comment.
Why not use the proper log4j way of doing things - basically replace %s for {}, no String#format(String) needed.
There was a problem hiding this comment.
@jamierocks There does not appear to be an interface method that takes both a Throwable and params.
There was a problem hiding this comment.
I believe that log4j knows to use the last parameter as a Throwable when taking a series of other parameters; however, I cannot find a source.
There was a problem hiding this comment.
I would write it this way:
LogManager.getLogger("LaunchWrapper").error("Exception encountered attempting classloading of {}", name, e);Internally the logger creates a FormattedMessage which can take a Throwable as the last argument:
https://logging.apache.org/log4j/log4j-2.1/log4j-api/apidocs/org/apache/logging/log4j/message/FormattedMessage.html
There was a problem hiding this comment.
Tracing through it doesn't use that constructor from the varargs method, but it does appear poke was correct as it seems to scan it at org.apache.logging.log4j.message.ParameterizedMessage#parseArguments
Chaarlesss
changed the title
|
Why, why this isn't merged? |
7 participants
I just fix two bugs I found during my experiments