Hi Gary,
On 16.11.2024 17:41, Gary Gregory wrote:
[1]https://github.com/apache/logging-log4j2/issues/3143
This issue points to the merged PR
https://github.com/apache/logging-log4j2/pull/2936, a large change set with
zero tests. This isn't great since there are no tests to catch regressions
:-(
When PR #2936 was reviewed, bug #3143 wasn't known.
While PR #2936 (for the `2.25.x` release) is not affected by the bug,
the bug itself was caused by a backport of PR #2936 to the `2.24.x`
branch (see PR #2961[1]). The bug itself is due to the reachability
rules for Java local variables: a variable becomes unreachable **not**
at the end of the syntactic scope, but right after its last
**non-trivial** usage (see [2] for more details). That is not an easy
problem to catch in a review IMHO and certainly not something you can
easily test.
[1] https://github.com/apache/logging-log4j2/pull/2961
[2] https://shipilev.net/jvm/anatomy-quarks/8-local-var-reachability/
[2]https://github.com/apache/logging-log4j2/pull/3209
LGTM: If I apply the patch, the test is green, then when I revert the
'main' changes, the test fails. Nice work :-)
[3]https://github.com/apache/logging-log4j2/pull/3210
LGTM: If I apply the patch, the 2 tests are green, then when I revert the
'main' changes, the 2 tests fail. Nice work :-)
So it all looks good :-)
Could you formally approve those PRs? Thanks.
Piotr
