-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[pigeon] Creates new Generator classes for each language #2996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
df851d5
e40d679
8c4e2a0
bcdbdbc
31bb701
06315ce
5675c64
a319b48
6d7405d
72a380c
1e59fef
0964cd3
f589da4
637679d
68a23e2
67282cf
d08606c
90fcb67
ea0ec6c
6206bad
7c3d35c
ef0b71a
8bf2dfb
8c6abf2
93a5d1b
710d1a1
d189d11
daae569
88905e3
29decb9
b7abbe4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
## 5.0.0 | ||
|
||
* Creates new Generator classes for each language. | ||
|
||
## 4.2.15 | ||
|
||
* Relocates generator classes. (Reverted) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
import 'ast.dart'; | ||
import 'generator_tools.dart'; | ||
|
||
/// A superclass of generator classes. | ||
/// | ||
/// This provides the structure that is common across generators for different languages. | ||
abstract class Generator<T> { | ||
/// Generates files for specified language with specified [languageOptions] | ||
void generate( | ||
T languageOptions, Root root, StringSink sink, FileType fileType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for making the adjustment. I think having a Generator interface and maintaining the Adapter keeps the generator files simple. Everything looks good but one thing. I think I missed some discussion on
What do we think we gained by adding them? Now there is a grouping of similar generators: CppHeader and CppSource. That seems like it complicates the design with little benefit. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback. Would the inclusion of a This allows for generators that don't have multiple file types to ignore the Is there a better way to handle combining the generator classes without the use of a parameter similar to this implementation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The issue is that they aren't "similar" they are two fundamentally linked things that can't meaningfully operate on their own. To be useful, each one must be paired with a call to the other with identical inputs. And changes to the header generator almost always require exactly corresponding changes to the source generator or everything breaks. That's why I would like to see combined classes for all of the linked groups; the goal here is to move all of the generator logic into the subclasses, and inherently codependent logic should be in the same class. (Having two parallel classes, each with half the logic, feels like when someone has two Ivar arrays where the object indexes must match, instead of using a map or an array of structures. Technically it works, but it's really fragile and harder to understand.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working on a design doc for this to allow for more effective communication about our goals here. https://docs.google.com/document/d/1ABr5yZM_sOZTd66bIVpsCJ7_koKmH0tZuLH4ZT1cs8c/edit?usp=sharing |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Why not make this a file type as well so the structure is the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering that, I just didn't think about it when I wrote the code. I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with this, upon thinking about it more, is that it would generate the test files when generating non test dart code as well. I don't know if that is super important to avoid, but it seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to avoid that by making the set of types option-driven, but let's not worry about it now and just leave it as you have it; we can always revisit later and it's a minor thing.