On Tue, 29 Apr 2025 14:04:07 GMT, Nikita Gubarkov <ngubar...@openjdk.org> wrote:
>> J2dTrace macros have multiple overloads specifying number of arguments,
>> making it less convent to change number of arguments. There were cases when
>> existing macros were not enough and people had to add new variants with even
>> more arguments. We could simply use variadic macros instead.
>>
>> Also, currently those macros expand to a { code block }, which doesn't
>> require a semicolon at the end, so it can sometimes be missed, leading to an
>> inconsistent code style. We could expand it directly to the function,
>> forcing user to insert a semicolon after that, in a function-like style.
>
> Nikita Gubarkov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fixup! 8355904: Use variadic macros for J2dTrace
Wrapped arguments were aligned to the opening parenthesis of the macro, thus
these lines need adjusting.
Bump the copyright year in all the modified files, please.
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/EncoderManager.m line
366:
> 364: J2dTraceLn(J2D_TRACE_VERBOSE,
> 365: "end common encoder because of dest change: %p -> %p",
> 366: _destination, dest);
Suggestion:
J2dTraceLn(J2D_TRACE_VERBOSE,
"end common encoder because of dest change: %p -> %p",
_destination, dest);
Restore aligning of the arguments on the wrapped lines.
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/EncoderManager.m line
379:
> 377: J2dTraceLn(J2D_TRACE_VERBOSE,
> 378: "end common encoder because toggle stencil: %d -> %d",
> 379: (int)_useStencil, (int)[_mtlc.clip isShape]);
Suggestion:
J2dTraceLn(J2D_TRACE_VERBOSE,
"end common encoder because toggle stencil: %d -> %d",
(int)_useStencil, (int)[_mtlc.clip isShape]);
Restore aligning. This applies to other files.
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line
186:
> 184: @autoreleasepool {
> 185: J2dTraceLn(J2D_TRACE_VERBOSE, "replaceTextureRegion src (dw, dh)
> : [%d, %d] dest (dx1, dy1) =[%d, %d]",
> 186: dw, dh, dx1, dy1);
Suggestion:
J2dTraceLn(J2D_TRACE_VERBOSE, "replaceTextureRegion src (dw, dh) : [%d,
%d] dest (dx1, dy1) =[%d, %d]",
dw, dh, dx1, dy1);
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBufImgOps.m line 50:
> 48: self = [super init];
> 49: if (self) {
> 50: J2dTraceLn(J2D_TRACE_INFO,"Created MTLRescaleOp:
> isNonPremult=%d", isNonPremult);
Suggestion:
J2dTraceLn(J2D_TRACE_INFO, "Created MTLRescaleOp: isNonPremult=%d",
isNonPremult);
Does it look reasonable to add a space after the first argument too?
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLContext.m line 254:
> 252: }
> 253:
> 254: J2dTraceLn(J2D_TRACE_VERBOSE, "MTLContext_SetSurfaces: bsrc=%p
> (tex=%p type=%d), bdst=%p (tex=%p type=%d)", srcOps, srcOps->pTexture,
> srcOps->drawableType, dstOps, dstOps->pTexture, dstOps->drawableType);
This long line is worth wrapping:
Suggestion:
J2dTraceLn(J2D_TRACE_VERBOSE, "MTLContext_SetSurfaces: bsrc=%p (tex=%p
type=%d), bdst=%p (tex=%p type=%d)",
srcOps, srcOps->pTexture, srcOps->drawableType,
dstOps, dstOps->pTexture, dstOps->drawableType);
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLContext.m line 360:
> 358:
> 359: - (void)setColorPaint:(int)pixel {
> 360: J2dTraceLn(J2D_TRACE_INFO, "MTLContext.setColorPaint: pixel=%08x
> [r=%d g=%d b=%d a=%d]", pixel, (pixel >> 16) & (0xFF), (pixel >> 8) & 0xFF,
> (pixel) & 0xFF, (pixel >> 24) & 0xFF);
Wrap the line, too?
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGlyphCache.m line
295:
> 293: J2dTraceLn(J2D_TRACE_VERBOSE,
> 294: " glyph 0x%x: removing cell 0x%x from glyph's
> list",
> 295: glyph, currCellInfo);
Suggestion:
J2dTraceLn(J2D_TRACE_VERBOSE,
" glyph 0x%x: removing cell 0x%x from glyph's list",
glyph, currCellInfo);
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLRenderQueue.h line
85:
> 83: if ((value) == NULL) { \
> 84: J2dTraceLn(J2D_TRACE_ERROR, \
> 85: "%s is null", #value); \
Suggestion:
J2dTraceLn(J2D_TRACE_ERROR, \
"%s is null", #value); \
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLRenderQueue.h line
94:
> 92: if ((value)) { \
> 93: J2dTraceLn(J2D_TRACE_ERROR, \
> 94: "%s is false", #value);\
Suggestion:
J2dTraceLn(J2D_TRACE_ERROR, \
"%s is false", #value); \
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceData.m line
96:
> 94: bmtlsdo->drawableType = rtt ? MTLSD_RT_TEXTURE : MTLSD_TEXTURE;
> 95:
> 96: J2dTraceLn(J2D_TRACE_VERBOSE, "MTLSurfaceData_initTexture: w=%d
> h=%d bp=%p [tex=%p] opaque=%d rtt=%d", width, height, bmtlsdo,
> bmtlsdo->pTexture, isOpaque, rtt);
Wrap the line?
src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLVertexCache.m line
224:
> 222:
> 223: J2dTraceLn(J2D_TRACE_INFO, "MTLVertexCache_AddMaskQuad: %d",
> 224: maskCacheIndex);
Suggestion:
J2dTraceLn(J2D_TRACE_INFO, "MTLVertexCache_AddMaskQuad: %d",
maskCacheIndex);
src/java.desktop/share/native/common/java2d/opengl/OGLContext.c line 756:
> 754: J2dRlsTraceLn(J2D_TRACE_INFO,
> 755: "OGLContext_IsLCDShaderSupportAvailable: not enough tex units
> (%d)",
> 756: maxTexUnits);
Suggestion:
J2dRlsTraceLn(J2D_TRACE_INFO,
"OGLContext_IsLCDShaderSupportAvailable: not enough tex units (%d)",
maxTexUnits);
However, continuation lines are usually indented by 8 spaces instead of 4
spaces.
src/java.desktop/share/native/common/java2d/opengl/OGLContext.c line 986:
> 984: J2dRlsTraceLn(J2D_TRACE_WARNING,
> 985: "OGLContext_CreateFragmentProgram: compiler msg (%d):\n%s",
> 986: infoLogLength, infoLog);
Suggestion:
J2dRlsTraceLn(J2D_TRACE_WARNING,
"OGLContext_CreateFragmentProgram: compiler msg (%d):\n%s",
infoLogLength, infoLog);
Aligning is weird here, I guess it's fine to amend this.
src/java.desktop/share/native/common/java2d/opengl/OGLContext.c line 1018:
> 1016: J2dRlsTraceLn(J2D_TRACE_WARNING,
> 1017: "OGLContext_CreateFragmentProgram: linker msg (%d):\n%s",
> 1018: infoLogLength, infoLog);
Suggestion:
J2dRlsTraceLn(J2D_TRACE_WARNING,
"OGLContext_CreateFragmentProgram: linker msg (%d):\n%s",
infoLogLength, infoLog);
src/java.desktop/share/native/common/java2d/opengl/OGLRenderQueue.h line 118:
> 116: if ((value) == NULL) { \
> 117: J2dTraceLn(J2D_TRACE_ERROR, \
> 118: "%s is null", #value); \
Suggestion:
J2dTraceLn(J2D_TRACE_ERROR, \
"%s is null", #value); \
src/java.desktop/windows/native/libawt/java2d/d3d/D3DContext.cpp line 1380:
> 1378: pd3dDevice->GetTransform(D3DTS_WORLD, &tx);
> 1379: J2dTraceLn(10,
> 1380: " %5f %5f %5f %5f", tx._11, tx._12, tx._13,
> tx._14);
I wonder why these lines use 10 as the level instead of constant. Should we
update these lines to use a trace constant/define?
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24949#pullrequestreview-2832814532
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084428158
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084429615
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084433163
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084438108
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084440921
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084442259
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084442917
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084444730
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084445157
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084448036
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084450001
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084501289
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084502360
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084503017
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084507075
PR Review Comment: https://git.openjdk.org/jdk/pull/24949#discussion_r2084517205
