RFR: 8267312: Resolve should use generated diagnostic factories

[Vicente Romero]

On Wed, 26 May 2021 14:09:39 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> > Some general comments about the generated code. I wonder if it would make sense to change the implementation of the toType() method which will fold common cases in the switch expression into a default case or generate a case label like: `case Type1, Type2 -> sameAction`.
> 
> I'll think about this - my first reaction is that the current strategy makes templating easier, but perhaps there's another way - e.g. by having a template for a single CASE statement, which can be parameterized on a number of labels.

if templating is easier the way it is in your current proposal, I would keep it that way

> 
> > I wonder if what we really want to model is one factory that can fold both implementations into one. I know this is generated code which should be ready to use, but just thinking aloud, not sure if there are some abstractions that could be useful from the client code perspective. I wonder if we could build on method DiagnosticInfo::of to define one stop factories. But I guess that you already considered this option.
> 
> I guess what you are suggesting is that, instead of having a method for converting a diagnostic info to a different one (like we do now) we should have a method to create a diagnostic info with the right kind from the start.
> 
> This is definitively an option - one of the issues is that the current generated file is divided by kinds (e.g. CompilerProperties has nested classes like Errors, Warnings, Notes) - so if we added such factories, they'd have to live at the top level (e.g. CompilerProperties). If that's acceptable I can do that. To be clear, the proposed structure will end up like this:
> 
> ```
> class CompilerProperties {
>       static class Errors {
>              static DiagnosticInfo ProbFoundReq(...) = ... // like before this patch
>              ...
>        }
>        static class Fragments {
>              static DiagnosticInfo ProbFoundReq(...) = ... // like before this patch
>              ...
>        }
>        
>        // shared factories
>        
>        static DiagnosticInfo ProbFoundReq(DiagnosticType type, args...) {
>            return switch (type) {
>                 case ERROR -> Errors.ProbFoundReq(args);
>                 case MISC -> Fragments.ProbFoundReq(args);
>                 default -> throw new AssertionError();
>             };
>        }
> }
> ```
> 
> This would solve the problem you mention, and also avoid a redundant allocation in Resolve.java.

right I like the shared factories proposal, I think the generated code will be simpler

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

PR: https://git.openjdk.java.net/jdk/pull/4089