Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14261002:
Make generator function data path more clear. (Closed)
|
Created:
7 years, 8 months ago by kojih Modified:
7 years, 8 months ago CC:
blink-reviews, Nate Chapin, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionMake generator function data path more clear.
Currently almost all function pushes generated code to global vars.
In this change, generated code is pushed by certain stuff(definition or declaration) instead of line by line.
Due to that, now NamedPropertyGetter is generated by 1 function.
Though it doesn't reduce the amount of code much, it improve readability and maintenability of the code generator.
There remains still several work, for example namespace clearance.
R=haraken@chromium.org
BUG=229740
TEST=V8TestEventTarget.cpp (generated by run-bindings-tests)
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148606
Patch Set 1 #Patch Set 2 : revised #Patch Set 3 : revised #Patch Set 4 : revised #Patch Set 5 : revised #Patch Set 6 : revised #
Total comments: 18
Patch Set 7 : revised #
Total comments: 8
Patch Set 8 : updated #Patch Set 9 : rebased to latest #
Messages
Total messages: 21 (0 generated)
r?
This looks like an ad-hoc fix that just increases a mess of the code generator.
I want to solve the problem in a more organized manner, even if it requires a
substantial amount of refactoring.
The core problem is that you can do nothing but sequentially adding code to
@implContent. To solve the problem, how about killing @implContent and instead
introducing @implFunctions, which stores a list of functions to be generated?
Each item in @implFunctions is a function, and all the functions are generated
at the very end of the code generation step. This way, you don't need to care
about whether you are in a function or not, which will significantly improve
readability of the code generator.
Specifically, in case of namedPropertyGetter(), the code will look like this:
sub GenerateImplementation {
my $configureTemplateFunction = "";
$configureTemplateFunction .= ...;
$configureTemplateFunction .= GenerateImplementationNamedPropertyGetter();
$configureTemplateFunction .= ...;
push(@implFunctions, $configureTemplateFunction);
}
sub GenerateImplementationNamedPropertyGetter {
my $codeFragment;
$codeFragment .= ...;
my $namedPropertyGetterFunction = "void namedPropertyGetter() { ... }";
push(@implFunctions, $configureTemplateFunction);
return $codeFragment;
}
WDYT? (I haven't yet investigated how much the idea will clean up the code
generator in practice.)
I think this is trade-off of generalization between specialization. I concern excessive specialization reduces flexibility of generator, but your idea sounds reasonable. @implFunction is not enough in case of generating not function(such as define or variables,...) to global scope. In that case, I think it's good to add @implDeclarations or something.
s/between/and/
> @implFunction is not enough in case of generating not function(such as define or > variables,...) to global scope. > In that case, I think it's good to add @implDeclarations or something. Sounds reasonable to me. Although it might not reduce the amount of code much, it will improve readability and maintenability of the code generator.
r?
now GenerateImplementationNamedPropertyGetter have 2 array ref arguments for output generated code. This is because this func must return registering namedPropertyGetter statement, and namedPropertyGetter itself.
It can return tuple ($codeA, $codeB). it might be one's preference.
The patch looks like just a mechanical replacement and doesn't much improve the current mess. Basically, we want to clean up the current situation where we're generating code sequentially (and thus we cannot do nothing but adding code just like document.write():-). We should change it so that (1) functions and declarations to be generated are constructed into organized structures, and (2) generate code from the structures at the final phase. How about preparing the following global variables to represent the structures? - @implFunctions (A list of strings. Each string is one function.) - @implInternalFunctions (A list of strings. Each string is one function.) - @implDeclarations (A list of strings. Each string is one statement.) By using one string for one function, we can remove a bunch of meaningless push(@..., "...") s. WDYT?
I don't understand difference between your idea and this implementation.
This patch already hold 1 function in 1 string in @implStuff.
@implStuff ... A list of strings. each string is one {function / variable}
{definition / declaration}
In this patch a function is generated like:
my @func = ();
push(@func, "...");
push(@func, "...");
push(@implStuff, join "", @func);
Do you mean it should be as below?
my $func = "";
$func .= "...";
$func .= "...";
push(@implStuff, $func);
r? 1. replace push(@code, XXX to $code .= XXX 2. replace output args -> return tuple 3. add utility method such as AddToImplFunction
https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... File Source/bindings/scripts/CodeGeneratorV8.pm (right): https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:37: my @implStuff = (); Shall we rename it to @implContent ? Stuff sounds ambiguous. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:38: my @implInternalStuff = (); @implContentInternals ? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:120: sub AddToImplStuffInternal AddToImplContentInternals ? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:126: sub AddToImplFunctionInternal Maybe this helper method is not needed, because we're not planning to distinguish functions from others, as discussed offline. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:132: sub AddToImplStuff AddToImplContent ? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:138: sub AddToImplFunction Ditto. We might not need this helper method. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:218: my $func = <<END; The Blink coding style prefers a full word for a variable name (http://www.chromium.org/blink/coding-style#TOC-Names). This should be $function. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:900: Nit: You might want to remove the trailing empty lines. You can add empty lines when you generate the code. You can do it in a follow-up patch. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:1311: my $out = ""; Maybe $code is a better name? Either way, let's make variable names consistent. For example: - Use $function for building up code for a function. - Use $code for building up code that is part of any function. WDYT? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2813: $namedPropertydGetter .= <<END; Can't you do this like AddToImplContent(<<END) ? Then you can avoid returning $namedPropertydGetter from this method. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2874: return ($tmplConfig, $namedPropertydGetter); Ditto. We want to avoid returning $namedPropertydGetter. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2917: my $implContentGlobal = []; Is this needed? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:3146: my $stuff = ""; $stuff is too ambiguous. $code ? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:3435: AddToImplFunction(join "", $namedPropertydGetter); You might want to do it in GenerateImplementationNamedPropertyGetter(). Would it be hard for some dependency reason?
updated. r? https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... File Source/bindings/scripts/CodeGeneratorV8.pm (right): https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:900: OK. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:1311: my $out = ""; as discussed offline: basically use $code use $subCode if variable name will conflict https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2813: $namedPropertydGetter .= <<END; Great, I did it. https://codereview.chromium.org/14261002/diff/17001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2917: my $implContentGlobal = []; no, removed.
LGTM. abarth: Do you have any ideas to make the code building step better? https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... File Source/bindings/scripts/CodeGeneratorV8.pm (right): https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:116: my @code = @_; my $code = shift; ? https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:1298: my $out = ""; $out => $code https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2600: my $out = ""; $out => $code https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:2688: my $out = ""; $out => $code https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:3585: Remove these empty lines. https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:3795: AddToImplContent($code); You don't need this. https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:4070: $result .= $subCode; $result => $code
got LGTM but updated. https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... File Source/bindings/scripts/CodeGeneratorV8.pm (right): https://codereview.chromium.org/14261002/diff/22001/Source/bindings/scripts/C... Source/bindings/scripts/CodeGeneratorV8.pm:116: my @code = @_; there's a code: AddToHeader(GenerateHeaderContentHeader($interface)); GenerateHeaderContentHeader returns array of string. But it's bit confused, so I changed return value of GenerateHeaderContentHeader to string.
I confirmed no $out, $func, $stuff, $result in the code.
abarth: Would you take a rough look at the patch? I'd like to ask your preference about how to make the code generation step more readable.
I'm not sure this CL is much of an improvement on the current code. I agree with haraken that we'd be better off with a more structured internal representation. The current design is as follows: IDL text -> IDL tree -> Impl text A more structured approach would be to add another representation: IDL text -> IDL tree -> Impl tree -> Impl text For example, the Impl tree would have nodes for each function we want to generate, and then within each function node we'd have nodes for processing each input argument, etc. This approach is somewhat standard in compilers where the abstract syntax tree of the language is translated into an intermediate data structure that is manipulated and then finally serialized as object code. I'm not sure whether we want to add this intermediate state now, but it's one approach we could consider for the future, especially if we switch the code generator to another language (like Python) that supports richer data structures more easily than Perl.
Thanks abarth. I agree with you. OK, so I'd like to LGTM the patch, since the patch doesn't make the current situation worse and this kind of fix will anyway be needed to auto-generate indexed/named property getters/setters. We can consider introducing more structured internal representation in the future.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kojih@chromium.org/14261002/31001
Message was sent while issue was closed.
Change committed as 148606 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
