Improve JPMS module opener#11269
Conversation
amarziali
force-pushed
the
andrea.marziali/jpms-followups
branch
2 times, most recently
from
c938acd to
2844e24
Compare
amarziali
force-pushed
the
andrea.marziali/jpms-followups
branch
from
2844e24 to
caf1bee
Compare
amarziali
added
type: enhancement
amarziali
requested review from
dougqh,
jbachorik and
sarahchen6
and removed request for
a team
|
| Suite | Status |
|---|---|
| Startup | |
| Load | |
| DaCapo |
Commit: d2f0ae67 · CI Pipeline · Benchmarking Platform UI
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caf1beed1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
amarziali
added
the
tag: ai generated
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mcculls
left a comment
mcculls
left a comment
There was a problem hiding this comment.
Overall looks ok, but some of the suggested changes to instrumenter classes don't make sense given how the methods will be called. triggerClasses() is only ever called once, so it doesn't make sense to move its return value to a constant even though the result does not change. If we applied this pattern to all instrumenter methods then we'd end up with bloated constant pools capturing one-shot return values.
…strap/instrumentation/java/module/JpmsHelper.java Co-authored-by: Stuart McCulloch <stuart.mcculloch@datadoghq.com>
…strap/instrumentation/java/module/JpmsHelper.java Co-authored-by: Stuart McCulloch <stuart.mcculloch@datadoghq.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
andrea.marziali/jpms-followups
branch
2 participants
What Does This Do
Got a code review feedback by claude on a previous PR in which the JPMS module opener was introduced. I let it do some cleanup and migrate a couple of tests to junit.
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.