Since the discussion happened over the holiday weekend, I didn't get a
chance to respond until now, but I think that this came to a good
outcome. As Alan's archaeology discovered, this flag appears to be a
leftover from the original implementation, and I could find no signs of
its usage. We might consider deprecating it (though, we might also
leave sleeping dogs alone), but its good to have a test for the current
behavior.
Allow me to make a few meta-comments about how we got here, though,
before we wrap up.
they are not visible via java.lang.reflect API. I assume that's just an
omission.
While omissions do sometimes happen, it is usually not the best first
theory in a situation like this. More often than not, there is a good,
but often non-obvious, explanation.
More importantly, though, the PR-first approach is not how we like to
approach such a change, especially when it involves the behaviors of
such fundamental APIs such as reflection, because it jumps over the most
important steps:
- What problem are we trying to solve?
- Is it really a problem that needs to be solved?
- Is it really a problem that needs to be solved in the JDK?
- What solutions are there, besides the obvious one?
- What are the pros, cons, and tradeoffs of the various solutions?
- Of the proposed solution, what future downsides and risks could we
imagine?
Going straight to the code of a specific solution is likely to be
unsatisfying in both the short term (because its the wrong conversation)
or the long term (because it might not be the right solution to the
right problem.) Instead, starting with a discussion of the problem, and
of potential solutions and tradeoffs, is likely to yield a better result
on both counts.
(As an example of the last one, suppose we committed this patch. One
could easily imagine some library later saying "must be run with
-Xx:PreserveAllAnnotations" because that's what the author had to do to
make it work. But a behavior change like non-runtime annotations
showing up unexpectedly in reflection could confuse existing code, which
might not work as expected with the new behavior. Which might then call
for "can we please make the PreserveAllAnnotations flag more selective
(e.g., per-class/module/package)" or calls for new flags or ... yuck.
We try to avoid today's solutions becoming tomorrow's problems. This is
not an exact science, of course, but this is the sort of stewardship we
strive for.)
On 6/1/2021 5:39 AM, Jaroslav Tulach wrote:
There doesn't seem to be much support for the complete changes in #4245. To get
at least something useful from that endeavor I have extracted the test for
existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this
pull request without any changes to the JVM behavior.
-------------
Commit messages:
- Test long time existing behavior of -XX:+PreserveAllAnnotations
Changes: https://git.openjdk.java.net/jdk/pull/4280/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4280&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8267936
Stats: 172 lines in 1 file changed: 172 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/4280.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4280/head:pull/4280
PR: https://git.openjdk.java.net/jdk/pull/4280
