On Tue, 25 May 2021 19:27:35 GMT, Vicente Romero <vrom...@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.
> 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.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4089
