On Wed, 26 May 2021 14:09:39 GMT, Maurizio Cimadamore <mcimadam...@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
