RFR: 8353757: Log class should have a proper clear() method [v2]

[Archie Cobbs]

On Mon, 7 Apr 2025 08:57:45 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Invoke Log.useSource() before recursing into attribution.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTrees.java line 364:
> 
>> 362:                 try {
>> 363:                     Env<AttrContext> env = getAttrContext(path.getTreePath());
>> 364:                     JavaFileObject prevSource = log.useSource(env.toplevel.sourcefile);
> 
> These fixes are independent correct? (I'm ok with them being added here)

Oops, you're right - those snuck in from an unrelated change. Fixed in 23447164d.

> The thing I'm unsure about is the `diagnosticHandler` field. I suppose we always call "clear" from a context where we already have popped all the installed alternate handlers -- but if not we should reset that too?

I was unsure as well. I think it is more correct to reset that field as well, but doing so raises the question: Will that change or break any existing code?

In order to gain confidence that it won't, I ran a bunch of tests with this patch applied:

--- i/src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
+++ w/src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
@@ -699,6 +699,7 @@ public void clear() {
         nwarnings = 0;
         nsuppressederrors = 0;
         nsuppressedwarns = 0;
+        Assert.check(diagnosticHandler.prev == null);
     }
 
     /**

and didn't get any crashes. So I think that means it's safe to add this (because for now it's must effectively be a no-op):

index e9da2efea9c..2167f471104 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
@@ -699,6 +699,8 @@ public void clear() {
         nwarnings = 0;
         nsuppressederrors = 0;
         nsuppressedwarns = 0;
+        while (diagnosticHandler.prev != null)
+            popDiagnosticHandler(diagnosticHandler);
     }
 
     /**

This is done in a9d584508.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24460#discussion_r2031388696
PR Review Comment: https://git.openjdk.org/jdk/pull/24460#discussion_r2031388886