Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1508293004:
Simplify Go's Mojom Type Generator (Closed)
|
Created:
5 years ago by alexfandrianto Modified:
4 years, 11 months ago CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org, yzshen+mojopublicwatch_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionSimplify Go's Mojom Type Generator
Removes all dependence on the identifier_store.
Originally, the identifier_store appeared to be needed because types were generated recursively, and their children types also needed to be registered. In order to break cycles and avoid repeats, we would use the identifier_store.
However, this is actually not needed. Each relevant type to be registered (enum/union/struct/interface) is already known beforehand by the generating Python script. Types belonging to other .mojom files are also registered explicitly. Since children types do NOT need to be registered, the identifier_store could be safely removed.
This CL also shifts the generation flag from the go generator to the generator scripts. This is a better location since it allows users to control whether types are generated or not.
BUG=https://github.com/domokit/mojo/issues/596
R=azani@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/a241773121e1588ad04f5d970a6ccc42ad52bc00
Patch Set 1 #Patch Set 2 : Upload again to trigger trybot tests #Patch Set 3 : Trigger Trybot tests again #
Total comments: 8
Patch Set 4 : Rebased and minor comment update #Patch Set 5 : Add generate_type_info flag to mojom generators #
Total comments: 15
Patch Set 6 : Address Code Cleanups #Patch Set 7 : Add arg instead of leaving it unparsed #Patch Set 8 : Add store_true to the argparse #
Total comments: 7
Patch Set 9 : Use hyphens in flag, not underscores #
Messages
Total messages: 21 (4 generated)
Description was changed from ========== Simplify Go's Mojom Type Generator Removes all dependence on the identifier_store. Shifts the generation flag to an earlier file. BUG= ========== to ========== Simplify Go's Mojom Type Generator Removes all dependence on the identifier_store. Shifts the generation flag to an earlier file. BUG= ==========
alexfandrianto@google.com changed reviewers: + azani@chromium.org, rudominer@chromium.org
thanks for the cl. https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:314: # Note: InterfaceRequestKind's should use the Interface inside them. The type is just InterfaceRequest. https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--go_gen_types" in args: Let's have a single flag control type generation in all languages for which it is implemented.
Thanks. I'm actually not quite sure what to do about the flag name. Currently, each generator only receives a subset of the remaining_args, so it is difficult to pass a common argument to all generators. (See comment and https://github.com/domokit/mojo/blob/master/mojo/public/tools/bindings/mojom_...) https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:314: # Note: InterfaceRequestKind's should use the Interface inside them. On 2015/12/10 23:16:18, azani wrote: > The type is just InterfaceRequest. Done. https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--go_gen_types" in args: On 2015/12/10 23:16:18, azani wrote: > Let's have a single flag control type generation in all languages for which it > is implemented. I very much agree, but I am having issues with lines 141 to 144 of the mojom_bindings_generator_v1.py, which is why it is called "--go_gen_types" instead of "--gen_types". - only remaining_args are allowed to be sent over to the generator. - of those, the args must begin with the generator's prefix - if there is no prefix, then no args are sent over In other words, if we leave that code there as-is, we cannot have a single common argument sent to each language's generator. I've found a workaround and can create an issue for this, but what would you recommend?
Can you please add a comment to the CL description explaining why you thought use of the identifier store was important before and why you no longer need it. https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--go_gen_types" in args: On 2015/12/11 01:26:57, alexfandrianto wrote: > On 2015/12/10 23:16:18, azani wrote: > > Let's have a single flag control type generation in all languages for which it > > is implemented. > > I very much agree, but I am having issues with lines 141 to 144 of the > mojom_bindings_generator_v1.py, which is why it is called "--go_gen_types" > instead of "--gen_types". > > - only remaining_args are allowed to be sent over to the generator. > - of those, the args must begin with the generator's prefix > - if there is no prefix, then no args are sent over > > In other words, if we leave that code there as-is, we cannot have a single > common argument sent to each language's generator. I've found a workaround and > can create an issue for this, but what would you recommend? Let's just leave it as you have it for now. We can fix it after we get rid of v1. https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... File mojo/public/tools/bindings/mojom_bindings_generator_v1.py (right): https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/mojom_bindings_generator_v1.py:211: # flags in. https://github.com/domokit/mojo/issues/558 The caller of this script is mojom_bindings_generator.py. Can you not put the this logic there? We want this flag for mojom_bindings_generator_v2 also because we want the tests to continue passing when use_new_mojom_compiler=true.
Hey Alex, Sorry this took so long. I thought I had responded to your CL days ago... https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--go_gen_types" in args: On 2015/12/11 01:26:57, alexfandrianto wrote: > On 2015/12/10 23:16:18, azani wrote: > > Let's have a single flag control type generation in all languages for which it > > is implemented. > > I very much agree, but I am having issues with lines 141 to 144 of the > mojom_bindings_generator_v1.py, which is why it is called "--go_gen_types" > instead of "--gen_types". > > - only remaining_args are allowed to be sent over to the generator. > - of those, the args must begin with the generator's prefix > - if there is no prefix, then no args are sent over > > In other words, if we leave that code there as-is, we cannot have a single > common argument sent to each language's generator. I've found a workaround and > can create an issue for this, but what would you recommend? I just chatted with Mitch and we came up with a solution. What you can do is after line 206 in mojom_bindings_generator_v1.py add an argument generate-type-info. Then, on line 144, check args.generate_type_info and if it is present, add it to filtered_args. Then, do the same thing in run_code_generators.py. Add the arg at line 41 and check it at line 165.
https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--go_gen_types" in args: On 2015/12/14 22:20:01, azani wrote: > On 2015/12/11 01:26:57, alexfandrianto wrote: > > On 2015/12/10 23:16:18, azani wrote: > > > Let's have a single flag control type generation in all languages for which > it > > > is implemented. > > > > I very much agree, but I am having issues with lines 141 to 144 of the > > mojom_bindings_generator_v1.py, which is why it is called "--go_gen_types" > > instead of "--gen_types". > > > > - only remaining_args are allowed to be sent over to the generator. > > - of those, the args must begin with the generator's prefix > > - if there is no prefix, then no args are sent over > > > > In other words, if we leave that code there as-is, we cannot have a single > > common argument sent to each language's generator. I've found a workaround and > > can create an issue for this, but what would you recommend? > > I just chatted with Mitch and we came up with a solution. What you can do is > after line 206 in mojom_bindings_generator_v1.py add an argument > generate-type-info. Then, on line 144, check args.generate_type_info and if it > is present, add it to filtered_args. > > Then, do the same thing in run_code_generators.py. Add the arg at line 41 and > check it at line 165. Thanks, that was very helpful. I added --generate_type_info and --no_generate_type_info with default to true (for compilation purposes.
Description was changed from ========== Simplify Go's Mojom Type Generator Removes all dependence on the identifier_store. Shifts the generation flag to an earlier file. BUG= ========== to ========== Simplify Go's Mojom Type Generator Removes all dependence on the identifier_store. Originally, the identifier_store appeared to be needed because types were generated recursively, and their children types also needed to be registered. In order to break cycles and avoid repeats, we would use the identifier_store. However, this is actually not needed. Each relevant type to be registered (enum/union/struct/interface) is already known beforehand by the generating Python script. Types belonging to other .mojom files are also registered explicitly. Since children types do NOT need to be registered, the identifier_store could be safely removed. This CL also shifts the generation flag from the go generator to the generator scripts. This is a better location since it allows users to control whether types are generated or not. BUG=https://github.com/domokit/mojo/issues/596 ==========
On 2015/12/15 20:53:40, alexfandrianto wrote: > https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... > File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): > > https://codereview.chromium.org/1508293004/diff/40001/mojo/public/tools/bindi... > mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if > "--go_gen_types" in args: > On 2015/12/14 22:20:01, azani wrote: > > On 2015/12/11 01:26:57, alexfandrianto wrote: > > > On 2015/12/10 23:16:18, azani wrote: > > > > Let's have a single flag control type generation in all languages for > which > > it > > > > is implemented. > > > > > > I very much agree, but I am having issues with lines 141 to 144 of the > > > mojom_bindings_generator_v1.py, which is why it is called "--go_gen_types" > > > instead of "--gen_types". > > > > > > - only remaining_args are allowed to be sent over to the generator. > > > - of those, the args must begin with the generator's prefix > > > - if there is no prefix, then no args are sent over > > > > > > In other words, if we leave that code there as-is, we cannot have a single > > > common argument sent to each language's generator. I've found a workaround > and > > > can create an issue for this, but what would you recommend? > > > > I just chatted with Mitch and we came up with a solution. What you can do is > > after line 206 in mojom_bindings_generator_v1.py add an argument > > generate-type-info. Then, on line 144, check args.generate_type_info and if it > > is present, add it to filtered_args. > > > > Then, do the same thing in run_code_generators.py. Add the arg at line 41 and > > check it at line 165. > > Thanks, that was very helpful. I added --generate_type_info and > --no_generate_type_info with default to true (for compilation purposes. PTAL, this is ready for review.
Thanks again. Sorry I didn't get to this sooner. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/go_templates/mojom_reference_macros.tmpl (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/go_templates/mojom_reference_macros.tmpl:29: {# No need to register anything #} Maybe clarify in the comment what types you would expect here that would not need to be registered. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:16: I think you can remove the GENERATOR_PREFIX now. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:313: # Use the kind's module to determine the package and name. Can you put this comment in the fuction's docstring? def GetIdentifier(kind): """Use the kind's module to determine the package and name.""" https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:322: # This is for a type that should not be stored. Return an empty identifier. I'm a little uncomfortable with this function promising to return the identifier of a kind but occasionally returning an empty string. Could you document when this will happen? (and maybe rename the function to something a bit more descriptive) It seems to me we should raise an error here. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--generate_type_info" in args: self.should_gen_mojom_types = "--generate_type_info" in args https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_java_generator.py:500: (args, _) = parser.parse_known_args(unparsed_args) Why is this change here?
Thanks for the review. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/go_templates/mojom_reference_macros.tmpl (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/go_templates/mojom_reference_macros.tmpl:29: {# No need to register anything #} On 2015/12/18 18:12:58, azani wrote: > Maybe clarify in the comment what types you would expect here that would not > need to be registered. Done. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:16: On 2015/12/18 18:12:58, azani wrote: > I think you can remove the GENERATOR_PREFIX now. Done. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:313: # Use the kind's module to determine the package and name. On 2015/12/18 18:12:58, azani wrote: > Can you put this comment in the fuction's docstring? > > def GetIdentifier(kind): > """Use the kind's module to determine the package and name.""" Done. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:322: # This is for a type that should not be stored. Return an empty identifier. On 2015/12/18 18:12:58, azani wrote: > I'm a little uncomfortable with this function promising to return the identifier > of a kind but occasionally returning an empty string. Could you document when > this will happen? (and maybe rename the function to something a bit more > descriptive) It seems to me we should raise an error here. Yeah, we should raise an error. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:389: if "--generate_type_info" in args: On 2015/12/18 18:12:58, azani wrote: > self.should_gen_mojom_types = "--generate_type_info" in args Done. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_java_generator.py:500: (args, _) = parser.parse_known_args(unparsed_args) On 2015/12/18 18:12:58, azani wrote: > Why is this change here? This change is here because the new argument will cause parse_args to throw (since it is unrecognized).
https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_go_generator.py:322: # This is for a type that should not be stored. Return an empty identifier. On 2015/12/18 19:08:31, alexfandrianto wrote: > On 2015/12/18 18:12:58, azani wrote: > > I'm a little uncomfortable with this function promising to return the > identifier > > of a kind but occasionally returning an empty string. Could you document when > > this will happen? (and maybe rename the function to something a bit more > > descriptive) It seems to me we should raise an error here. > > Yeah, we should raise an error. Maybe you forgot to commit. It's still returning an empty string. https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_java_generator.py:500: (args, _) = parser.parse_known_args(unparsed_args) On 2015/12/18 19:08:31, alexfandrianto wrote: > On 2015/12/18 18:12:58, azani wrote: > > Why is this change here? > > This change is here because the new argument will cause parse_args to throw > (since it is unrecognized). Can you add an argument to parser instead?
On 2015/12/18 23:19:11, azani wrote: > https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... > File mojo/public/tools/bindings/generators/mojom_go_generator.py (right): > > https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... > mojo/public/tools/bindings/generators/mojom_go_generator.py:322: # This is for a > type that should not be stored. Return an empty identifier. > On 2015/12/18 19:08:31, alexfandrianto wrote: > > On 2015/12/18 18:12:58, azani wrote: > > > I'm a little uncomfortable with this function promising to return the > > identifier > > > of a kind but occasionally returning an empty string. Could you document > when > > > this will happen? (and maybe rename the function to something a bit more > > > descriptive) It seems to me we should raise an error here. > > > > Yeah, we should raise an error. > > Maybe you forgot to commit. It's still returning an empty string. > > https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... > File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): > > https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... > mojo/public/tools/bindings/generators/mojom_java_generator.py:500: (args, _) = > parser.parse_known_args(unparsed_args) > On 2015/12/18 19:08:31, alexfandrianto wrote: > > On 2015/12/18 18:12:58, azani wrote: > > > Why is this change here? > > > > This change is here because the new argument will cause parse_args to throw > > (since it is unrecognized). > > Can you add an argument to parser instead? Are you looking at the latest patchset? I may have responded just before uploading it. Was that wrong?
https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/mojom_java_generator.py:500: (args, _) = parser.parse_known_args(unparsed_args) On 2015/12/18 23:19:11, azani wrote: > On 2015/12/18 19:08:31, alexfandrianto wrote: > > On 2015/12/18 18:12:58, azani wrote: > > > Why is this change here? > > > > This change is here because the new argument will cause parse_args to throw > > (since it is unrecognized). > > Can you add an argument to parser instead? Done.
On 2015/12/18 23:24:10, alexfandrianto wrote: > https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... > File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): > > https://codereview.chromium.org/1508293004/diff/80001/mojo/public/tools/bindi... > mojo/public/tools/bindings/generators/mojom_java_generator.py:500: (args, _) = > parser.parse_known_args(unparsed_args) > On 2015/12/18 23:19:11, azani wrote: > > On 2015/12/18 19:08:31, alexfandrianto wrote: > > > On 2015/12/18 18:12:58, azani wrote: > > > > Why is this change here? > > > > > > This change is here because the new argument will cause parse_args to throw > > > (since it is unrecognized). > > > > Can you add an argument to parser instead? > > Done. (Fixed the error with argparse.) PTAL
Sorry, I forgot to send my drafts a couple hours ago. https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... File mojo/public/tools/bindings/mojom_bindings_generator_v1.py (right): https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/mojom_bindings_generator_v1.py:209: parser.add_argument("--generate_type_info", dest="generate_type_info", Same as in the other file. https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... File mojo/public/tools/bindings/run_code_generators.py (right): https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/run_code_generators.py:42: parser.add_argument("--generate_type_info", dest="generate_type_info", Make the argument name "generate-type-info" to match the other args. https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/run_code_generators.py:48: parser.set_defaults(generate_type_info=True) Just use the default param to add_argument. Also, don't we want to default to False?
https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... File mojo/public/tools/bindings/mojom_bindings_generator_v1.py (right): https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/mojom_bindings_generator_v1.py:209: parser.add_argument("--generate_type_info", dest="generate_type_info", On 2015/12/21 22:28:37, azani wrote: > Same as in the other file. Done. https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... File mojo/public/tools/bindings/run_code_generators.py (right): https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/run_code_generators.py:42: parser.add_argument("--generate_type_info", dest="generate_type_info", On 2015/12/21 22:28:37, azani wrote: > Make the argument name "generate-type-info" to match the other args. Done. https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/run_code_generators.py:48: parser.set_defaults(generate_type_info=True) On 2015/12/21 22:28:37, azani wrote: > Just use the default param to add_argument. Also, don't we want to default to > False? The default is true so that the test(s) will compile. action="store_true" and "store_false" already have the opposite as defaults. I like this way more since it's clearer what it will end up being. Further, it is also clearer how to change it in the future.
lgtm https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... File mojo/public/tools/bindings/run_code_generators.py (right): https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... mojo/public/tools/bindings/run_code_generators.py:48: parser.set_defaults(generate_type_info=True) On 2015/12/21 23:50:11, alexfandrianto wrote: > On 2015/12/21 22:28:37, azani wrote: > > Just use the default param to add_argument. Also, don't we want to default to > > False? > > The default is true so that the test(s) will compile. > > action="store_true" and "store_false" already have the opposite as defaults. I > like this way more since it's clearer what it will end up being. Further, it is > also clearer how to change it in the future. I understand your point regarding tests, but if we set it to true by default, this means type info will be generated for everybody. As long as this is just something that happens with go bindings, that's not a big deal because you're the primary users. But the Dart bindings are in use by a lot of people and they shouldn't have to do anything to avoid generating extraneous code. So I'm ok with this for now, but we need to switch the default to False before your dart bindings change is landed.
Description was changed from ========== Simplify Go's Mojom Type Generator Removes all dependence on the identifier_store. Originally, the identifier_store appeared to be needed because types were generated recursively, and their children types also needed to be registered. In order to break cycles and avoid repeats, we would use the identifier_store. However, this is actually not needed. Each relevant type to be registered (enum/union/struct/interface) is already known beforehand by the generating Python script. Types belonging to other .mojom files are also registered explicitly. Since children types do NOT need to be registered, the identifier_store could be safely removed. This CL also shifts the generation flag from the go generator to the generator scripts. This is a better location since it allows users to control whether types are generated or not. BUG=https://github.com/domokit/mojo/issues/596 ========== to ========== Simplify Go's Mojom Type Generator Removes all dependence on the identifier_store. Originally, the identifier_store appeared to be needed because types were generated recursively, and their children types also needed to be registered. In order to break cycles and avoid repeats, we would use the identifier_store. However, this is actually not needed. Each relevant type to be registered (enum/union/struct/interface) is already known beforehand by the generating Python script. Types belonging to other .mojom files are also registered explicitly. Since children types do NOT need to be registered, the identifier_store could be safely removed. This CL also shifts the generation flag from the go generator to the generator scripts. This is a better location since it allows users to control whether types are generated or not. BUG=https://github.com/domokit/mojo/issues/596 R=azani@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/a241773121e1588ad04f5d970a6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as a241773121e1588ad04f5d970a6ccc42ad52bc00 (presubmit successful).
Message was sent while issue was closed.
On 2016/01/04 22:20:44, azani wrote: > lgtm > > https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... > File mojo/public/tools/bindings/run_code_generators.py (right): > > https://codereview.chromium.org/1508293004/diff/140001/mojo/public/tools/bind... > mojo/public/tools/bindings/run_code_generators.py:48: > parser.set_defaults(generate_type_info=True) > On 2015/12/21 23:50:11, alexfandrianto wrote: > > On 2015/12/21 22:28:37, azani wrote: > > > Just use the default param to add_argument. Also, don't we want to default > to > > > False? > > > > The default is true so that the test(s) will compile. > > > > action="store_true" and "store_false" already have the opposite as defaults. I > > like this way more since it's clearer what it will end up being. Further, it > is > > also clearer how to change it in the future. > > I understand your point regarding tests, but if we set it to true by default, > this means type info will be generated for everybody. As long as this is just > something that happens with go bindings, that's not a big deal because you're > the primary users. But the Dart bindings are in use by a lot of people and they > shouldn't have to do anything to avoid generating extraneous code. So I'm ok > with this for now, but we need to switch the default to False before your dart > bindings change is landed. Thanks. This is definitely a point that we will need to reconcile. Depending on our priorities, it may actually not be that bad to allow the extra code to be generated in Dart. For the next CL, I will try to get numbers on how the code size is affected by type generation. If the increase in size is not acceptable, we will have to work something out. Since a Dart test for Mojom types will have to run too, it's not as simple as having the default be false for Dart. We will need to come up with a better solution. If the solution we come up with is nontrivial, then it can still be better to temporarily ( a few weeks?) accept an increase in code size since a code size increase is not very harmful to Dart users. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
