Skip to content

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Jul 19, 2021

@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 3 times, most recently from 2794aff to cad8fc3 Compare July 26, 2021 09:57
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch from 0f4bbe7 to 59fd6e3 Compare July 26, 2021 17:22
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 8 times, most recently from 24bf7f7 to b52baa0 Compare August 25, 2021 21:22
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 4 times, most recently from fbfcc1d to 3cf466e Compare August 26, 2021 17:37
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch from 3cf466e to 0673858 Compare August 27, 2021 15:52
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 3 times, most recently from bf01286 to d32b050 Compare August 31, 2021 08:12
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 3 times, most recently from de7bc85 to 395a9b2 Compare September 3, 2021 15:07
Comment on lines 50 to 41
writer.append("\"predicate\":{");
writer.append("\"whenTypeReachable\":\"").append(predicate.getTypeName()).append("\"");
writer.append("},").newline();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "predicate": declaration is a part of the type object and should be printed by TypeConfiguration. Only the {"whenTypeReachable":"..."} part should be printed here.
Please use the JsonWriter.quote for readability and safety with special characters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you propose to have redundant checks if the printing of predicated should happen? Then we should duplicate that if and the printing of predicate in all different configuration kinds. This is hardly minimal as ConfigurationPredicate will be uniform in all configuration kinds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, we should simply rename the method to printPredicateAttribute then. Also, I meant ConfigurationType (not TypeConfiguration) in my comment above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method is still called ConfigurationPredicatePrintable.printJson in your last commit.

@vjovanov
Copy link
Member

vjovanov commented Sep 7, 2021

I am starting to consider changing whenTypeReachable to typeReachable. I think when does not add much to the meaning, but adds to the size of the file. With this change we would have

{
  "predicate" : { "typeReachable" : "io.netty.util.internal.PlatformDependent0" },
  "name" : "sun.misc.Unsafe",
  "fields" : [
    { "name" : "theUnsafe" }
  ]
}

instead of

{
  "predicate" : { "whenTypeReachable" : "io.netty.util.internal.PlatformDependent0" },
  "name" : "sun.misc.Unsafe",
  "fields" : [
    { "name" : "theUnsafe" }
  ]
}

WDYT?

@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 2 times, most recently from 3f1e7d2 to 2eeabe0 Compare September 7, 2021 07:17
@peter-hofer
Copy link
Member

peter-hofer commented Sep 7, 2021

I am starting to consider changing whenTypeReachable to typeReachable. I think when does not add much to the meaning, but adds to the size of the file.

I think this is better regardless of file size concerns. You could consider changing predicate to just if, too -- it comes more natural to programmers anyway. It might be mistaken for an abbreviation though, so perhaps condition is better.

@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 2 times, most recently from 3644e5b to 659fecf Compare September 7, 2021 10:47
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch from 659fecf to 01a8fa7 Compare September 7, 2021 11:37
@vjovanov
Copy link
Member

vjovanov commented Sep 7, 2021

Agree, also predicate is something that is evaluated over an argument and yields a boolean. Here the relation to the argument is unclear. So we are left with:

{
  "condition" : { "typeReachable" : "io.netty.util.internal.PlatformDependent0" },
  "name" : "sun.misc.Unsafe",
  "fields" : [
    { "name" : "theUnsafe" }
  ]
}

or

{
  "if" : { "typeReachable" : "io.netty.util.internal.PlatformDependent0" },
  "name" : "sun.misc.Unsafe",
  "fields" : [
    { "name" : "theUnsafe" }
  ]
}

or

{
  "when" : { "typeReachable" : "io.netty.util.internal.PlatformDependent0" },
  "name" : "sun.misc.Unsafe",
  "fields" : [
    { "name" : "theUnsafe" }
  ]
}

@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 3 times, most recently from ce41138 to fc4db98 Compare September 8, 2021 09:27
@peter-hofer
Copy link
Member

Agree, also predicate is something that is evaluated over an argument and yields a boolean. Here the relation to the argument is unclear. So we are left with:

I like condition best, because in my opinion it feels most natural to a reader that the entire entry is conditional on the specified expression, and it is a noun like most other attribute names. With if and when, there is sort of a lingering question of then what?

@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 2 times, most recently from 1f2c9e1 to 70044a1 Compare September 8, 2021 10:07
@vjovanov
Copy link
Member

vjovanov commented Sep 8, 2021

I like condition the most as well. I think it is a safe bet here and fits in the code well. I'll make a PR on top of serialization, and then I'll squash them all with the first commit message refactored.

@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch 6 times, most recently from f2429f5 to 5b3b8ce Compare September 8, 2021 15:17
Introducing conditions for reflection, JNI, and serialization configuration. The condition
can be only a type name. For example,
"condition": {
  "typeReachable": "org.graalvm.ReachableClass"
 },

will apply configuration only if type "org.graalvm.ReachableClass" is reachable.
The default condition for every reflection entry is "java.lang.Object" and as
such it will not be printed.

The Native Image agent does not emit conditions, but will fuse conditions
accordingly. Instead all config of one type, the fusion will happen by
the condition and the type.
@graalvmbot graalvmbot force-pushed the github/vjovanov/GR-32340-predicated-configuration branch from 5b3b8ce to 80c9ff8 Compare September 8, 2021 15:42
@graalvmbot graalvmbot merged commit adfb1fe into master Sep 10, 2021
@graalvmbot graalvmbot deleted the github/vjovanov/GR-32340-predicated-configuration branch February 8, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants