Reorder j9javaaccessflags.h and remove unused flags #17788

theresa-m · 2023-07-13T20:41:49Z

Reorganized these values to be more readable
Deleted the unused values:
#define J9AccMandated 0x8000
#define J9AccClassReferenceShift 0x1C
#define J9AccClassUnused400 0x400 <-- I don't think these placeholders are really helping anyone
#define J9AccClassUnused200 0x200

Related to #17785

runtime/oti/j9javaaccessflags.h

hangshao0 · 2023-07-14T15:56:32Z

runtime/oti/j9javaaccessflags.h

-#define J9AccStrict 0x800
-#define J9AccSuper 0x20
-#define J9AccSynchronized 0x20
-#define J9AccSynthetic 0x1000


If J9AccSynthetic is removed from ROMClass->extraModifiers, you may want to modify this comment as well:

openj9/runtime/bcutil/ROMClassBuilder.cpp

Line 1183 in f7305e7

* + AccSynthetic (matches Oracle modifier position)

This comment has not been addressed.
Also, the indentation on line 1176 is wrong.

Please correct the indentation of line 1176 of ROMClassBuilder.cpp (there should be one more space before +).

I'd still like to see this fixed.

@theresa-m ?

Hey sorry I missed this, it should be fixed now.

runtime/oti/j9javaaccessflags.h

theresa-m · 2023-08-01T17:35:25Z

@keithc-ca do you have any more review comments for this?

keithc-ca · 2023-08-01T21:33:46Z

@keithc-ca do you have any more review comments for this?

I was waiting for the changes you discussed in #17788 (comment) to appear here.

theresa-m · 2023-08-02T14:32:15Z

Sorry - forgot to push. They should be there now.

hangshao0 · 2023-08-02T14:55:22Z

Can you include the change for this comment #17788 (comment) as well ?

runtime/oti/j9javaaccessflags.h

keithc-ca · 2023-08-02T16:25:07Z

runtime/oti/j9javaaccessflags.h

-#define J9AccStrict 0x800
-#define J9AccSuper 0x20
-#define J9AccSynchronized 0x20
-#define J9AccSynthetic 0x1000


This comment has not been addressed.
Also, the indentation on line 1176 is wrong.

hangshao0 · 2023-08-02T18:36:58Z

runtime/bcutil/j9bcu.tdf

@@ -76,7 +76,6 @@ TraceException=Trc_BCU_createRomClassEndian_CopyMethodsError Noenv Overhead=1 Le
 TraceException=Trc_BCU_createRomClassEndian_CopyCPShapeError Noenv Overhead=1 Level=3 Template="BCU j9bcutil_createROMClassEndian: error occurred during copyCPShapeAndPreinitDataToROM. error=%d"
 TraceException=Trc_BCU_createRomClassEndian_AnnotationError2 Noenv Overhead=1 Level=3 Template="BCU j9bcutil_createROMClassEndian: error occurred during buildAnnotationInfo. error=%d"
 TraceException=Trc_BCU_createRomClassEndian_DebugInfoError2 Noenv Overhead=1 Level=3 Template="BCU j9bcutil_createROMClassEndian: error occurred during buildDebugInfo. error=%d"
-TraceEvent=Trc_BCU_createRomClassEndian_Synthetic Noenv Overhead=1 Level=3 Template="BCU j9bcutil_createROMClassEndian: Setting J9AccSynthetic flag on ROMClass %p"


You can mark this tracepoint as Obsolete. We cannot remove trace point. Each trace point has an index with it. Removing this will change the index number of all the tracepoints below.

See https://github.com/eclipse-openj9/openj9/blob/master/doc/diagnostics/AddingTracepoints.md#rules-for-adding-tracepoint-definitions-to-tdf-files.

runtime/bcutil/ROMClassBuilder.cpp

runtime/oti/j9javaaccessflags.h

- Reorganized these values to be more readable - Deleted the unused values: define J9AccMandated 0x8000 define J9AccClassReferenceShift 0x1C define J9AccClassUnused400 0x400 <-- I don't think these placeholders are really helping anyone define J9AccClassUnused200 0x200 Signed-off-by: Theresa Mammarella <[email protected]>

runtime/oti/j9javaaccessflags.h

keithc-ca · 2023-08-16T17:45:52Z

runtime/oti/j9javaaccessflags.h

+#define J9AccForwarderMethod               0x00002000 /* method */
+#define J9AccEnum                          0x00004000 /* class field */
+#define J9AccEmptyMethod                   0x00004000 /* method */
+/* ACC_MODULE reserves 0x8000 for classes*/


Please insert a space before */.

As well as the missing space, perhaps 0x8000 should be 0x00008000 for consistency.

keithc-ca · 2023-08-16T17:46:58Z

I also draw your attention to my comment (https://github.com/eclipse-openj9/openj9/pull/17788/files#r1296068401) requesting a small fix to ROMClassBuilder.cpp.

runtime/oti/j9javaaccessflags.h

keithc-ca · 2023-08-16T19:04:23Z

runtime/oti/j9javaaccessflags.h

+#define J9AccForwarderMethod               0x00002000 /* method */
+#define J9AccEnum                          0x00004000 /* class field */
+#define J9AccEmptyMethod                   0x00004000 /* method */
+/* ACC_MODULE reserves 0x8000 for classes*/


As well as the missing space, perhaps 0x8000 should be 0x00008000 for consistency.

keithc-ca · 2023-08-16T19:04:39Z

runtime/oti/j9javaaccessflags.h

-#define J9AccStrict 0x800
-#define J9AccSuper 0x20
-#define J9AccSynchronized 0x20
-#define J9AccSynthetic 0x1000


@theresa-m ?

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m · 2023-08-16T19:09:10Z

Thanks @keithc-ca, I have made changes from your latest comments.

keithc-ca · 2023-08-16T19:25:00Z

Jenkins test sanity alinux64 jdk21

theresa-m force-pushed the refactoraccessflags branch 2 times, most recently from 45a4865 to 8b79242 Compare July 13, 2023 20:48

theresa-m changed the title ~~Refactoraccessflags~~ Reorder j9javaaccessflags.h and remove unused flags Jul 13, 2023

theresa-m force-pushed the refactoraccessflags branch 2 times, most recently from 8cbc6c9 to f69d42d Compare July 13, 2023 20:52

theresa-m requested a review from keithc-ca July 13, 2023 20:58

keithc-ca reviewed Jul 14, 2023

View reviewed changes

runtime/oti/j9javaaccessflags.h Outdated Show resolved Hide resolved

keithc-ca reviewed Jul 14, 2023

View reviewed changes

runtime/oti/j9javaaccessflags.h Outdated Show resolved Hide resolved

theresa-m force-pushed the refactoraccessflags branch 2 times, most recently from 9847cd3 to 1ca2932 Compare July 14, 2023 14:47

hangshao0 reviewed Jul 14, 2023

View reviewed changes

keithc-ca reviewed Jul 14, 2023

View reviewed changes

runtime/oti/j9javaaccessflags.h Outdated Show resolved Hide resolved

theresa-m requested a review from keithc-ca July 20, 2023 18:25

theresa-m force-pushed the refactoraccessflags branch from 1ca2932 to 59e474a Compare August 2, 2023 14:31

theresa-m force-pushed the refactoraccessflags branch from 59e474a to fbc30d3 Compare August 2, 2023 15:44

keithc-ca reviewed Aug 2, 2023

View reviewed changes

hangshao0 reviewed Aug 2, 2023

View reviewed changes

keithc-ca reviewed Aug 2, 2023

View reviewed changes

runtime/bcutil/ROMClassBuilder.cpp Outdated Show resolved Hide resolved

theresa-m force-pushed the refactoraccessflags branch from 4a4e118 to ce16de1 Compare August 3, 2023 15:03

hangshao0 approved these changes Aug 3, 2023

View reviewed changes

keithc-ca reviewed Aug 10, 2023

View reviewed changes

runtime/oti/j9javaaccessflags.h Outdated Show resolved Hide resolved

theresa-m force-pushed the refactoraccessflags branch 2 times, most recently from 7d5302d to 58be28c Compare August 16, 2023 14:00

keithc-ca requested changes Aug 16, 2023

View reviewed changes

runtime/oti/j9javaaccessflags.h Outdated Show resolved Hide resolved

keithc-ca mentioned this pull request Aug 16, 2023

Remove unused TEST_xxx macros #17966

Merged

theresa-m force-pushed the refactoraccessflags branch 2 times, most recently from 3250657 to 7e0b427 Compare August 16, 2023 17:29

keithc-ca reviewed Aug 16, 2023

View reviewed changes

keithc-ca requested changes Aug 16, 2023

View reviewed changes

theresa-m force-pushed the refactoraccessflags branch from 7e0b427 to e38881b Compare August 16, 2023 19:05

Label modifiers flags

a0f0aed

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m force-pushed the refactoraccessflags branch from e38881b to a0f0aed Compare August 16, 2023 19:08

keithc-ca approved these changes Aug 16, 2023

View reviewed changes

keithc-ca merged commit 06a3948 into eclipse-openj9:master Aug 16, 2023

theresa-m deleted the refactoraccessflags branch September 13, 2023 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder j9javaaccessflags.h and remove unused flags #17788

Reorder j9javaaccessflags.h and remove unused flags #17788

theresa-m commented Jul 13, 2023 •

edited

Loading

hangshao0 Jul 14, 2023

keithc-ca Aug 2, 2023

keithc-ca Aug 3, 2023 •

edited

Loading

keithc-ca Aug 16, 2023

keithc-ca Aug 16, 2023

theresa-m Aug 16, 2023

theresa-m commented Aug 1, 2023

keithc-ca commented Aug 1, 2023

theresa-m commented Aug 2, 2023

hangshao0 commented Aug 2, 2023

keithc-ca Aug 2, 2023

hangshao0 Aug 2, 2023

keithc-ca Aug 2, 2023

keithc-ca Aug 16, 2023

keithc-ca Aug 16, 2023

keithc-ca commented Aug 16, 2023

keithc-ca Aug 16, 2023

keithc-ca Aug 16, 2023

theresa-m commented Aug 16, 2023

keithc-ca commented Aug 16, 2023

Reorder j9javaaccessflags.h and remove unused flags #17788

Reorder j9javaaccessflags.h and remove unused flags #17788

Conversation

theresa-m commented Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresa-m commented Aug 1, 2023

keithc-ca commented Aug 1, 2023

theresa-m commented Aug 2, 2023

hangshao0 commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresa-m commented Aug 16, 2023

keithc-ca commented Aug 16, 2023

theresa-m commented Jul 13, 2023 •

edited

Loading

keithc-ca Aug 3, 2023 •

edited

Loading