[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: RFR: 8267312: Resolve should use generated diagnostic factories
From:       Vicente Romero <vromero () openjdk ! java ! net>
Date:       2021-05-27 15:59:03
Message-ID: XqVl1D6MEg9b49SQDwiMT4H1obAHQrcghxrrVR6U4qU=.6c1a01f8-d79f-4ff3-9501-7fdad2c2ce1e () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]