Skip to content

Conversation

@gnodet
Copy link
Contributor

@gnodet gnodet commented Mar 21, 2025

JIRA issue: MNG-8510

  • Rename o.a.m.cling.invoker.Utils to CliUtils
  • Move DefaultPluginXmlFactory into maven-impl and plugin xml reader/writers to maven-support
  • Make Utils package protected and rename them

@gnodet gnodet added this to the 4.0.0-rc-4 milestone Mar 21, 2025
import java.util.stream.Collectors;

class CoreUtils {
public static <T> T nonNull(T t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or should we use Objects.requireNonNull(T) instead? The exception is not the same (NullPointerException instead of IllegalArgumentException), but the former became the standard usage in the JDK API. It is also consistent with the code that just rely on implicit null-check, especially since Java 14 automatically generates an exception message with the name of the parameter that was null. Finally, Objects.requireNonNull(T) as some JVM-level optimization (the @ForceInline annotation) that we can't reproduce in our own null-check method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: the automatically generated message cited in above comment is for implicit null-checks, not for the exception thrown by Objects.requireNonNull(T). I would propose that when a method uses the parameter immediately like below:

public void foo(String biz) {
   if (biz.equals("foo")) {
   }
}

If biz is null, Java 14 and latter while automatically generates a message saying "Cannot invoke "String.equals(Object)" because "biz" is null". Therefore, it may be better (compared to no message at all) to not invoke a method such as nonNull if we accept that the exception is NullPointerException. For cases where the implicit null check would be too late, the Objects.requireNonNull methods throw an exception of type consistent with the implicit null checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agreed, Eliotte raised the same issue a while ago. Though the point of this PR was just to avoid the Utils class name clash, so we can create a subsequent PR to remove those custom methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also agreed. This PR is an improvement by itself, so merge it and then fix that.

@gnodet gnodet merged commit 09efae6 into apache:master Mar 24, 2025
13 checks passed
@jira-importer
Copy link

Resolve #9926

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