Skip to content

[pigeon] Creates writePrologue method on Generator classes #3029

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.

Sign up for GitHub

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

Closed
tarrinneal wants to merge 2 commits into from
Closed

[pigeon] Creates writePrologue method on Generator classes #3029

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9664a06
Creates writeHeader method on Generator classes
tarrinneal Jan 6, 2023
02fd734
prologue
tarrinneal Jan 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/pigeon/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.0.1

* Adds writeHeaders method to Generator classes and updates tests to use new Generators.

## 5.0.0

* Creates new Generator classes for each language.
Expand Down
56 changes: 41 additions & 15 deletions packages/pigeon/lib/cpp_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,33 @@ class CppGenerator extends Generator<OutputFileOptions<CppOptions>> {
/// Instantiates a Cpp Generator.
CppGenerator();

/// Generates Cpp files with specified [OutputFileOptions<CppOptions>]
/// Generates Cpp header files with specified [CppOptions]
@override
void generate(OutputFileOptions<CppOptions> languageOptions, Root root,
void generate(OutputFileOptions<CppOptions> generatorOptions, Root root,
StringSink sink) {
final FileType fileType = languageOptions.fileType;
final FileType fileType = generatorOptions.fileType;
assert(fileType == FileType.header || fileType == FileType.source);

final Indent indent = Indent(sink);
if (fileType == FileType.header) {
generateCppHeader(languageOptions.languageOptions, root, sink);
writeFileHeaders(generatorOptions, root, sink, indent);
generateCppHeader(generatorOptions.languageOptions, root, sink, indent);
} else {
generateCppSource(languageOptions.languageOptions, root, sink);
writeFileHeaders(generatorOptions, root, sink, indent);
generateCppSource(generatorOptions.languageOptions, root, sink, indent);
}
}

@override
void writeFileHeaders(OutputFileOptions<CppOptions> generatorOptions,
Root root, StringSink sink, Indent indent) {
final FileType fileType = generatorOptions.fileType;
if (fileType == FileType.header) {
writeCppHeaderHeader(
generatorOptions.languageOptions, root, sink, indent);
} else {
writeCppSourceHeader(
generatorOptions.languageOptions, root, sink, indent);
}
}
}
Expand Down Expand Up @@ -1049,18 +1066,23 @@ void _writeSystemHeaderIncludeBlock(Indent indent, List<String> headers) {
}
}

/// Generates the ".h" file for the AST represented by [root] to [sink] with the
/// provided [options] and [headerFileName].
void generateCppHeader(CppOptions options, Root root, StringSink sink) {
final String? headerFileName = options.headerOutPath;
final Indent indent = Indent(sink);
/// Writes Cpp header file header to sink.
void writeCppHeaderHeader(
CppOptions options, Root root, StringSink sink, Indent indent) {
if (options.copyrightHeader != null) {
addLines(indent, options.copyrightHeader!, linePrefix: '// ');
}
indent.writeln('$_commentPrefix $generatedCodeWarning');
indent.writeln('$_commentPrefix $seeAlsoWarning');
indent.addln('');
final String guardName = _getGuardName(headerFileName, options.namespace);
}

/// Generates the ".h" file for the AST represented by [root] to [sink] with the
/// provided [options] and [headerFileName].
void generateCppHeader(
CppOptions options, Root root, StringSink sink, Indent indent) {
final String guardName =
_getGuardName(options.headerIncludePath, options.namespace);
indent.writeln('#ifndef $guardName');
indent.writeln('#define $guardName');

Expand Down Expand Up @@ -1140,10 +1162,9 @@ void generateCppHeader(CppOptions options, Root root, StringSink sink) {
indent.writeln('#endif // $guardName');
}

/// Generates the ".cpp" file for the AST represented by [root] to [sink] with the
/// provided [options].
void generateCppSource(CppOptions options, Root root, StringSink sink) {
final Indent indent = Indent(sink);
/// Writes Cpp source file header to sink.
void writeCppSourceHeader(
CppOptions options, Root root, StringSink sink, Indent indent) {
if (options.copyrightHeader != null) {
addLines(indent, options.copyrightHeader!, linePrefix: '// ');
}
Expand All @@ -1152,7 +1173,12 @@ void generateCppSource(CppOptions options, Root root, StringSink sink) {
indent.addln('');
indent.addln('#undef _HAS_EXCEPTIONS');
indent.addln('');
}

/// Generates the ".cpp" file for the AST represented by [root] to [sink] with the
/// provided [options].
void generateCppSource(
CppOptions options, Root root, StringSink sink, Indent indent) {
indent.writeln('#include "${options.headerIncludePath}"');
indent.addln('');
_writeSystemHeaderIncludeBlock(indent, <String>[
Expand Down
75 changes: 45 additions & 30 deletions packages/pigeon/lib/dart_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,30 @@ class DartGenerator extends Generator<DartOptions> {

/// Generates Dart files with specified [DartOptions]
@override
void generate(DartOptions languageOptions, Root root, StringSink sink,
{FileType fileType = FileType.na}) {
assert(fileType == FileType.na);
generateDart(languageOptions, root, sink);
void generate(DartOptions generatorOptions, Root root, StringSink sink) {
final Indent indent = Indent(sink);

writeFileHeaders(generatorOptions, root, sink, indent);
generateDart(generatorOptions, root, sink, indent);
}

@override
void writeFileHeaders(
DartOptions generatorOptions, Root root, StringSink sink, Indent indent) {
writeHeader(generatorOptions, root, sink, indent);
}

/// Generates Dart files for testing with specified [DartOptions]
void generateTest(DartOptions languageOptions, Root root, StringSink sink) {
final Indent indent = Indent(sink);
final String sourceOutPath = languageOptions.sourceOutPath ?? '';
final String testOutPath = languageOptions.testOutPath ?? '';
writeTestHeader(languageOptions, root, sink, indent);
generateTestDart(
languageOptions,
root,
sink,
indent,
sourceOutPath: sourceOutPath,
testOutPath: testOutPath,
);
Expand Down Expand Up @@ -501,25 +511,26 @@ String _addGenericTypesNullable(TypeDeclaration type) {
return type.isNullable ? '$genericdType?' : genericdType;
}

/// Writes file header to sink.
void writeHeader(DartOptions opt, Root root, StringSink sink, Indent indent) {
if (opt.copyrightHeader != null) {
addLines(indent, opt.copyrightHeader!, linePrefix: '// ');
}
indent.writeln('// $generatedCodeWarning');
indent.writeln('// $seeAlsoWarning');
indent.writeln(
'// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import',
);
indent.addln('');
}

/// Generates Dart source code for the given AST represented by [root],
/// outputting the code to [sink].
void generateDart(DartOptions opt, Root root, StringSink sink) {
void generateDart(DartOptions opt, Root root, StringSink sink, Indent indent) {
final List<String> customClassNames =
root.classes.map((Class x) => x.name).toList();
final List<String> customEnumNames =
root.enums.map((Enum x) => x.name).toList();
final Indent indent = Indent(sink);

void writeHeader() {
if (opt.copyrightHeader != null) {
addLines(indent, opt.copyrightHeader!, linePrefix: '// ');
}
indent.writeln('// $generatedCodeWarning');
indent.writeln('// $seeAlsoWarning');
indent.writeln(
'// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import',
);
}

void writeEnums() {
for (final Enum anEnum in root.enums) {
Expand Down Expand Up @@ -678,7 +689,6 @@ $resultAt != null
}
}

writeHeader();
writeImports();
writeEnums();
for (final Class klass in root.classes) {
Expand Down Expand Up @@ -740,18 +750,9 @@ String _posixify(String inputPath) {
return context.fromUri(path.toUri(path.absolute(inputPath)));
}

/// Generates Dart source code for test support libraries based on the given AST
/// represented by [root], outputting the code to [sink]. [sourceOutPath] is the
/// path of the generated dart code to be tested. [testOutPath] is where the
/// test code will be generated.
void generateTestDart(
DartOptions opt,
Root root,
StringSink sink, {
required String sourceOutPath,
required String testOutPath,
}) {
final Indent indent = Indent(sink);
/// Writes file header to sink.
void writeTestHeader(
DartOptions opt, Root root, StringSink sink, Indent indent) {
if (opt.copyrightHeader != null) {
addLines(indent, opt.copyrightHeader!, linePrefix: '// ');
}
Expand All @@ -761,6 +762,20 @@ void generateTestDart(
'// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, unnecessary_import',
);
indent.writeln('// ignore_for_file: avoid_relative_lib_imports');
}

/// Generates Dart source code for test support libraries based on the given AST
/// represented by [root], outputting the code to [sink]. [dartOutPath] is the
/// path of the generated dart code to be tested. [testOutPath] is where the
/// test code will be generated.
void generateTestDart(
DartOptions opt,
Root root,
StringSink sink,
Indent indent, {
required String sourceOutPath,
required String testOutPath,
}) {
indent.writeln("import 'dart:async';");
indent.writeln(
"import 'dart:typed_data' show Float64List, Int32List, Int64List, Uint8List;",
Expand Down
22 changes: 20 additions & 2 deletions packages/pigeon/lib/generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,29 @@
// 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);
/// Generates files for specified language with specified [generatorOptions]
///
/// This method, when overridden, should follow a generic structure that is currently:
/// 1. Create Indent
/// 2. Write File Headers
/// 3. Generate File
void generate(
T generatorOptions,
Root root,
StringSink sink,
);

/// Adds specified file headers.
void writeFileHeaders(
Copy link
Member

@gaaclarke gaaclarke Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this correctly, the only client of writeFileHeaders is the implementations of generate, right? In that sense writeFileHeaders is not really an abstraction over Generator implementations that has any meaning. Generator implementations can just ignore that method. I don't see value in this change beyond the value of just splitting out the logic into it's own procedure, which I think we already had everywhere.

Now we could force it to have meaning by having pigeon_lib call into it. I don't know if that actually makes the code any clearer though.

I haven't had a chance to look at the other PRs you have for this cleanup but I think if there is more, it's worth creating a design doc that we can iterate on more quickly. I can help you with that if you want by drawing up where we are at now, then all you have to do is tweak the changes we are thinking of making.

Copy link
Member

@gaaclarke gaaclarke Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to Tarrin offline and we decided to circle back to his design doc on this refactor. After talking about it, this refactor sounds like a good approach generally but I'd like to:

  1. Make sure we agree on a final state (including naming and stuff)
  2. Incrementally build on the extraction of logic from the Generators with each PR

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to fully design and name every element of the final state in advance? The problem we have right now is that the generators are big soups of methods with no parallel structure (thus hard to maintain, and with no ability to leverage parallel structure).

The goal is to have a central structure of common elements. That lends itself extremely well to incremental, bottom-up extraction of clearly parallel parts (e.g., every generator needs to output each data class, so a method to write the data class is an obvious common element to extract; do that then rinse and repeat). A key advantage of that approach is that each step is simple and clear, and over time makes it much easier to see what high-level driving logic is fully the same, and which isn't, which will inform the final state.

It's much harder to see the right final states when we mostly have soup, so I'm not sure why we want to do all the design work at the point where it's hardest. What are the specific concerns with incremental extraction?

Copy link
Member

@gaaclarke gaaclarke Jan 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to turn code reviews into design reviews. The goal of what we are trying to accomplish is make the codebase have more form and less duplication. To be clear, I think we are on the right path generally.

The last PR I had to give some design feedback and that creates undue work on the contributor since they have to update code, if we are able to have those discussions before the code was written the process will go more smoothly and the results will be better. We have enough information about what the code is doing and what we want to accomplish we should be able to convey our design.

I've written up a design based on what Tarrin and I have discussed in PlantUML and gave him the source code so he can tweak it. I think that will be a good jumping off point for us all to get on the same page.

Copy link
Member

@gaaclarke gaaclarke Jan 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to have a central structure of common elements.

Here's one specific open design question, where does that shared structure should live:

  1. Docstrings and abstract (essentially private) methods in Generator? (current path)
  2. Pigeon.run()'s logic and the public interface for Generator?
  3. In an abstract base class which structured Generators inherit from? (the proposal I just made to Tarrin)

T generatorOptions,
Root root,
StringSink sink,
Indent indent,
);
}
4 changes: 2 additions & 2 deletions packages/pigeon/lib/generator_tools.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import 'dart:mirrors';
import 'ast.dart';

/// The current version of pigeon. This must match the version in pubspec.yaml.
const String pigeonVersion = '5.0.0';
const String pigeonVersion = '5.0.1';

/// Read all the content from [stdin] to a String.
String readStdin() {
Expand Down Expand Up @@ -510,7 +510,7 @@ enum FileType {
na,
}

/// Options for [Generator]'s that have multiple output file types.
/// Options for [Generator]s that have multiple output file types.
///
/// Specifies which file to write as well as wraps all language options.
class OutputFileOptions<T> {
Expand Down
40 changes: 24 additions & 16 deletions packages/pigeon/lib/java_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,17 @@ class JavaGenerator extends Generator<JavaOptions> {

/// Generates Java files with specified [JavaOptions]
@override
void generate(JavaOptions languageOptions, Root root, StringSink sink,
{FileType fileType = FileType.na}) {
assert(fileType == FileType.na);
generateJava(languageOptions, root, sink);
void generate(JavaOptions generatorOptions, Root root, StringSink sink) {
final Indent indent = Indent(sink);

writeFileHeaders(generatorOptions, root, sink, indent);
generateJava(generatorOptions, root, sink, indent);
}

@override
void writeFileHeaders(
JavaOptions generatorOptions, Root root, StringSink sink, Indent indent) {
writeHeader(generatorOptions, root, sink, indent);
}
}

Expand Down Expand Up @@ -536,22 +543,25 @@ String _castObject(
}
}

/// Writes file header to sink.
void writeHeader(
JavaOptions options, Root root, StringSink sink, Indent indent) {
if (options.copyrightHeader != null) {
addLines(indent, options.copyrightHeader!, linePrefix: '// ');
}
indent.writeln('// $generatedCodeWarning');
indent.writeln('// $seeAlsoWarning');
indent.addln('');
}

/// Generates the ".java" file for the AST represented by [root] to [sink] with the
/// provided [options].
void generateJava(JavaOptions options, Root root, StringSink sink) {
void generateJava(
JavaOptions options, Root root, StringSink sink, Indent indent) {
final Set<String> rootClassNameSet =
root.classes.map((Class x) => x.name).toSet();
final Set<String> rootEnumNameSet =
root.enums.map((Enum x) => x.name).toSet();
final Indent indent = Indent(sink);

void writeHeader() {
if (options.copyrightHeader != null) {
addLines(indent, options.copyrightHeader!, linePrefix: '// ');
}
indent.writeln('// $generatedCodeWarning');
indent.writeln('// $seeAlsoWarning');
}

void writeImports() {
indent.writeln('import android.util.Log;');
Expand Down Expand Up @@ -766,8 +776,6 @@ void generateJava(JavaOptions options, Root root, StringSink sink) {
}''');
}

writeHeader();
indent.addln('');
if (options.package != null) {
indent.writeln('package ${options.package};');
}
Expand Down
Loading