Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
56543003:
Add "implemented_in" key in IDL schema compiler (Closed)
|
Created:
7 years, 1 month ago by Haojian Wu Modified:
7 years, 1 month ago CC:
chromium-reviews, satorux1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd "implemented_in" key in IDL schema compiler
The "implemented_in" key format like this:
[implemented_in="path/to/implemented_file"]
namespace Foo {...};
BUG=313579
TEST=pass idl_schema_test
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233442
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address kalman's comment #
Total comments: 16
Patch Set 3 : Address sergeygs's comments. #Patch Set 4 : Update #Patch Set 5 : update #
Messages
Total messages: 27 (0 generated)
Please review it. Thanks.
sweet, thanks for adding this. https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:328: compiler_options={}): don't put objects as default values. they're shared amongst all instances of Namespace. just do = None. https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:389: compiler_options = {} =None? https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:398: compiler_options) name optional arguments. platforms=platforms, compiler_options=compiler_options https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:402: platforms = None reset compiler_options here? https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:415: compiler_options = dict(implemented_in=node.value) personally I would prefer {'implemented_in': node.value} rather than this syntax.
sweet, thanks for adding this.
https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:328: compiler_options={}): On 2013/11/02 00:01:52, kalman wrote: > don't put objects as default values. they're shared amongst all instances of > Namespace. just do = None. Done. https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:389: compiler_options = {} On 2013/11/02 00:01:52, kalman wrote: > =None? Done. https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:398: compiler_options) On 2013/11/02 00:01:52, kalman wrote: > name optional arguments. platforms=platforms, compiler_options=compiler_options Done. https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:402: platforms = None On 2013/11/02 00:01:52, kalman wrote: > reset compiler_options here? Done. https://codereview.chromium.org/56543003/diff/1/tools/json_schema_compiler/id... tools/json_schema_compiler/idl_schema.py:415: compiler_options = dict(implemented_in=node.value) On 2013/11/02 00:01:52, kalman wrote: > personally I would prefer {'implemented_in': node.value} rather than this > syntax. Done.
Thanks for submitting https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options else self.compiler_options} I'd prefer: 'compiler_options': self.compiler_options or {} It's a common idiom in Python and it's a lot simpler. Also, conditional expressions are restricted to one-liners in Google guide, while you have 2 lines (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:399: platforms=platforms, compiler_options=compiler_options) Align 'platform...' with 'node' in the preceding line. I think you will also have to move 'compiler_options' to new line then as well. Google style guide is silent about this, but PEP 8 is explicit: http://www.python.org/dev/peps/pep-0008/#indentation. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:417: compiler_options = {"implemented_in": node.value} Please use single quotes here: { 'implemented_in': node.value }. It's a frequent Python convention to use double quotes for plain language and/or interpolated strings, and single quotes for identifiers that would result in a logical or runtime error if misspelled. I can't say we stick to that in our code, but if anything, we favor single quotes, specifically in the surrounding code in this file and others in this CL. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/idl_schema_test.py (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema_test.py:136: expected = "idl_namespace_specific_implement.idl" Single quotes? https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema_test.py:144: expected_implemented_path = "idl_namespace_specific_implement_chromeos.idl" Single quotes? https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/idl_namespace_specific_implement.idl (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/idl_namespace_specific_implement.idl:7: [implemented_in="idl_namespace_specific_implement.idl"] Single quotes? https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/idl_namespace_specific_implement_chromeos.idl (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/idl_namespace_specific_implement_chromeos.idl:7: [platforms=("chromeos"), Single quotes?
Thanks for sergeygs's useful and detailed comments. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options else self.compiler_options} On 2013/11/02 06:20:44, sergeygs wrote: > I'd prefer: > > 'compiler_options': self.compiler_options or {} > > It's a common idiom in Python and it's a lot simpler. Also, conditional > expressions are restricted to one-liners in Google guide, while you have 2 lines > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) Done. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:399: platforms=platforms, compiler_options=compiler_options) On 2013/11/02 06:20:44, sergeygs wrote: > Align 'platform...' with 'node' in the preceding line. I think you will also > have to move 'compiler_options' to new line then as well. Google style guide is > silent about this, but PEP 8 is explicit: > http://www.python.org/dev/peps/pep-0008/#indentation. Done. Thanks for the link. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:417: compiler_options = {"implemented_in": node.value} On 2013/11/02 06:20:44, sergeygs wrote: > Please use single quotes here: { 'implemented_in': node.value }. It's a frequent > Python convention to use double quotes for plain language and/or interpolated > strings, and single quotes for identifiers that would result in a logical or > runtime error if misspelled. I can't say we stick to that in our code, but if > anything, we favor single quotes, specifically in the surrounding code in this > file and others in this CL. Done. Thanks for the detailed explainations. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/idl_schema_test.py (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema_test.py:136: expected = "idl_namespace_specific_implement.idl" On 2013/11/02 06:20:44, sergeygs wrote: > Single quotes? Done. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema_test.py:144: expected_implemented_path = "idl_namespace_specific_implement_chromeos.idl" On 2013/11/02 06:20:44, sergeygs wrote: > Single quotes? Done. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/idl_namespace_specific_implement.idl (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/idl_namespace_specific_implement.idl:7: [implemented_in="idl_namespace_specific_implement.idl"] On 2013/11/02 06:20:44, sergeygs wrote: > Single quotes? We cannot use single quotes as double quotes in IDL files since it's an illegal character in IDL parser. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/idl_namespace_specific_implement_chromeos.idl (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/idl_namespace_specific_implement_chromeos.idl:7: [platforms=("chromeos"), On 2013/11/02 06:20:44, sergeygs wrote: > Single quotes? The same.
On 2013/11/03 01:56:16, Haojian Wu wrote: > Thanks for sergeygs's useful and detailed comments. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > File tools/json_schema_compiler/idl_schema.py (right): > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options else > self.compiler_options} > On 2013/11/02 06:20:44, sergeygs wrote: > > I'd prefer: > > > > 'compiler_options': self.compiler_options or {} > > > > It's a common idiom in Python and it's a lot simpler. Also, conditional > > expressions are restricted to one-liners in Google guide, while you have 2 > lines > > > > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) > > Done. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/idl_schema.py:399: platforms=platforms, > compiler_options=compiler_options) > On 2013/11/02 06:20:44, sergeygs wrote: > > Align 'platform...' with 'node' in the preceding line. I think you will also > > have to move 'compiler_options' to new line then as well. Google style guide > is > > silent about this, but PEP 8 is explicit: > > http://www.python.org/dev/peps/pep-0008/#indentation. > > Done. Thanks for the link. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/idl_schema.py:417: compiler_options = > {"implemented_in": node.value} > On 2013/11/02 06:20:44, sergeygs wrote: > > Please use single quotes here: { 'implemented_in': node.value }. It's a > frequent > > Python convention to use double quotes for plain language and/or interpolated > > strings, and single quotes for identifiers that would result in a logical or > > runtime error if misspelled. I can't say we stick to that in our code, but if > > anything, we favor single quotes, specifically in the surrounding code in this > > file and others in this CL. > > Done. Thanks for the detailed explainations. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > File tools/json_schema_compiler/idl_schema_test.py (right): > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/idl_schema_test.py:136: expected = > "idl_namespace_specific_implement.idl" > On 2013/11/02 06:20:44, sergeygs wrote: > > Single quotes? > > Done. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/idl_schema_test.py:144: expected_implemented_path = > "idl_namespace_specific_implement_chromeos.idl" > On 2013/11/02 06:20:44, sergeygs wrote: > > Single quotes? > > Done. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > File tools/json_schema_compiler/test/idl_namespace_specific_implement.idl > (right): > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/test/idl_namespace_specific_implement.idl:7: > [implemented_in="idl_namespace_specific_implement.idl"] > On 2013/11/02 06:20:44, sergeygs wrote: > > Single quotes? > > We cannot use single quotes as double quotes in IDL files since it's an illegal > character in IDL parser. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > File > tools/json_schema_compiler/test/idl_namespace_specific_implement_chromeos.idl > (right): > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/test/idl_namespace_specific_implement_chromeos.idl:7: > [platforms=("chromeos"), > On 2013/11/02 06:20:44, sergeygs wrote: > > Single quotes? > > The same. LGTM
lgtm but consider my comment. https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options else self.compiler_options} On 2013/11/03 01:56:16, Haojian Wu wrote: > On 2013/11/02 06:20:44, sergeygs wrote: > > I'd prefer: > > > > 'compiler_options': self.compiler_options or {} > > > > It's a common idiom in Python and it's a lot simpler. Also, conditional > > expressions are restricted to one-liners in Google guide, while you have 2 > lines > > > > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) > > Done. The problem with "self.compiler_options or {}" is that the behaviour subtly changes if self._compiler_options is an empty object (vs None), because bool({}) is false. This is why being explicit in the object case is every-so-slighly safer, generally speaking. "foo or Foo()" is great when bool(Foo()) is true. If the two-line-conditional is a problem then assign it outside the loop.
On 2013/11/04 15:16:49, kalman wrote: > lgtm but consider my comment. > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > File tools/json_schema_compiler/idl_schema.py (right): > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options else > self.compiler_options} > On 2013/11/03 01:56:16, Haojian Wu wrote: > > On 2013/11/02 06:20:44, sergeygs wrote: > > > I'd prefer: > > > > > > 'compiler_options': self.compiler_options or {} > > > > > > It's a common idiom in Python and it's a lot simpler. Also, conditional > > > expressions are restricted to one-liners in Google guide, while you have 2 > > lines > > > > > > > > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) > > > > Done. > > The problem with "self.compiler_options or {}" is that the behaviour subtly > changes if self._compiler_options is an empty object (vs None), because bool({}) > is false. This is why being explicit in the object case is every-so-slighly > safer, generally speaking. "foo or Foo()" is great when bool(Foo()) is true. > > If the two-line-conditional is a problem then assign it outside the loop. To be honest, I can't see any change in the behavior here. "self.compiler_options or {}" is a direct translation of the original code: 'compiler_options': {} if not self.compiler_options else self.compiler_options "x or some_default" is dangerous when some_default is different from the "zero value" of the x's type ("{}" in this case), and your program must differentiate between x==None and "x==zero_value". Then you can fall into this trap, but not here.
https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/idl_namespace_specific_implement.idl (right): https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/idl_namespace_specific_implement.idl:7: [implemented_in="idl_namespace_specific_implement.idl"] Right, sorry: I got a little carried away with this. On 2013/11/03 01:56:16, Haojian Wu wrote: > On 2013/11/02 06:20:44, sergeygs wrote: > > Single quotes? > > We cannot use single quotes as double quotes in IDL files since it's an illegal > character in IDL parser.
On 2013/11/04 17:55:28, sergeygs wrote: > On 2013/11/04 15:16:49, kalman wrote: > > lgtm but consider my comment. > > > > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > > File tools/json_schema_compiler/idl_schema.py (right): > > > > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > > tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options > else > > self.compiler_options} > > On 2013/11/03 01:56:16, Haojian Wu wrote: > > > On 2013/11/02 06:20:44, sergeygs wrote: > > > > I'd prefer: > > > > > > > > 'compiler_options': self.compiler_options or {} > > > > > > > > It's a common idiom in Python and it's a lot simpler. Also, conditional > > > > expressions are restricted to one-liners in Google guide, while you have 2 > > > lines > > > > > > > > > > > > > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) > > > > > > Done. > > > > The problem with "self.compiler_options or {}" is that the behaviour subtly > > changes if self._compiler_options is an empty object (vs None), because > bool({}) > > is false. This is why being explicit in the object case is every-so-slighly > > safer, generally speaking. "foo or Foo()" is great when bool(Foo()) is true. > > > > If the two-line-conditional is a problem then assign it outside the loop. > > To be honest, I can't see any change in the behavior here. > "self.compiler_options or {}" is a direct translation of the original code: > > 'compiler_options': {} if not self.compiler_options else self.compiler_options > > "x or some_default" is dangerous when some_default is different from the "zero > value" of the x's type ("{}" in this case), and your program must differentiate > between x==None and "x==zero_value". Then you can fall into this trap, but not > here. Yes sorry. It needs to be "compiler_options if compiler_options is not None else {}"
On 2013/11/04 18:09:04, kalman wrote: > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > > > File tools/json_schema_compiler/idl_schema.py (right): > > > > > > > > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > > > tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options > > else > > > self.compiler_options} > > > On 2013/11/03 01:56:16, Haojian Wu wrote: > > > > On 2013/11/02 06:20:44, sergeygs wrote: > > > > > I'd prefer: > > > > > > > > > > 'compiler_options': self.compiler_options or {} > > > > > > > > > > It's a common idiom in Python and it's a lot simpler. Also, conditional > > > > > expressions are restricted to one-liners in Google guide, while you have > 2 > > > > lines > > > > > > > > > > > > > > > > > > > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) > > > > > > > > Done. > > > > > > The problem with "self.compiler_options or {}" is that the behaviour subtly > > > changes if self._compiler_options is an empty object (vs None), because > > bool({}) > > > is false. This is why being explicit in the object case is every-so-slighly > > > safer, generally speaking. "foo or Foo()" is great when bool(Foo()) is true. > > > > > > If the two-line-conditional is a problem then assign it outside the loop. > > > > To be honest, I can't see any change in the behavior here. > > "self.compiler_options or {}" is a direct translation of the original code: > > > > > 'compiler_options': {} if not self.compiler_options else > self.compiler_options > > > > "x or some_default" is dangerous when some_default is different from the "zero > > value" of the x's type ("{}" in this case), and your program must > differentiate > > between x==None and "x==zero_value". Then you can fall into this trap, but not > > here. > > Yes sorry. It needs to be "compiler_options if compiler_options is not None else > {}" From http://docs.python.org/2/library/stdtypes.html?highlight=short%20circuit#bool..., x or y is the same with: if x false then y else x So I think self.compiler_options or {} is ok at here. Either self.compiler_options is empty({}) or None, the evaluation of this expression is {} which is expected. Anyway I have changed it to if-else statement. About the two-line-conditional format, I find the same usage in chromium code repo, see here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... If you insist, I can move it outside.
On 2013/11/05 01:14:35, Haojian Wu wrote: > On 2013/11/04 18:09:04, kalman wrote: > > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > > > > File tools/json_schema_compiler/idl_schema.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/56543003/diff/70001/tools/json_schema_compile... > > > > tools/json_schema_compiler/idl_schema.py:364: if not self.compiler_options > > > else > > > > self.compiler_options} > > > > On 2013/11/03 01:56:16, Haojian Wu wrote: > > > > > On 2013/11/02 06:20:44, sergeygs wrote: > > > > > > I'd prefer: > > > > > > > > > > > > 'compiler_options': self.compiler_options or {} > > > > > > > > > > > > It's a common idiom in Python and it's a lot simpler. Also, > conditional > > > > > > expressions are restricted to one-liners in Google guide, while you > have > > 2 > > > > > lines > > > > > > > > > > > > > > > > > > > > > > > > > > > (https://engdoc.corp.google.com/eng/doc/pyguide.xml?showone=Conditional_Expres...) > > > > > > > > > > Done. > > > > > > > > The problem with "self.compiler_options or {}" is that the behaviour > subtly > > > > changes if self._compiler_options is an empty object (vs None), because > > > bool({}) > > > > is false. This is why being explicit in the object case is > every-so-slighly > > > > safer, generally speaking. "foo or Foo()" is great when bool(Foo()) is > true. > > > > > > > > If the two-line-conditional is a problem then assign it outside the loop. > > > > > > To be honest, I can't see any change in the behavior here. > > > "self.compiler_options or {}" is a direct translation of the original code: > > > > > > > > > 'compiler_options': {} if not self.compiler_options else > > self.compiler_options > > > > > > "x or some_default" is dangerous when some_default is different from the > "zero > > > value" of the x's type ("{}" in this case), and your program must > > differentiate > > > between x==None and "x==zero_value". Then you can fall into this trap, but > not > > > here. > > > > Yes sorry. It needs to be "compiler_options if compiler_options is not None > else > > {}" > > From > http://docs.python.org/2/library/stdtypes.html?highlight=short%2520circuit#bo..., > x or y is the same with: > if x false then y else x > > So I think self.compiler_options or {} is ok at here. Either > self.compiler_options is empty({}) or None, the evaluation > of this expression is {} which is expected. Anyway I have changed it to if-else > statement. The expression ({} is None) is False. Therefore if you have "x if foo is None else {}" then if foo is an {} then it's going to be foo, not a new {}. > > About the two-line-conditional format, I find the same usage in chromium code > repo, see here: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... Indeed. That looks like an error. > > If you insist, I can move it outside. Yes please.
> The expression ({} is None) is False. Therefore if you have "x if foo is None
> else {}" then if foo is an {} then it's going to be foo, not a new {}.
Yes, you're right. With your modified version of the code, the behavior is
indeed different from "x or y", and that can matter when it's important that the
returned object reference point at the original object (in this case
"this.compiler_options"), not one created on the fly. It's probably not the case
here, but again, LGTM either way.
On 2013/11/05 01:44:15, sergeygs wrote:
> > The expression ({} is None) is False. Therefore if you have "x if foo is
None
> > else {}" then if foo is an {} then it's going to be foo, not a new {}.
>
> Yes, you're right. With your modified version of the code, the behavior is
> indeed different from "x or y", and that can matter when it's important that
the
> returned object reference point at the original object (in this case
> "this.compiler_options"), not one created on the fly. It's probably not the
case
> here, but again, LGTM either way.
Oh, I see! Thanks for kalman@ and sergeygs@ explainations.
Done.
lgtm
Which parts do you need each of the three reviewers to review? I will assume my role is zero whenever I see multiple reviewers unless told otherwise.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/56543003/260001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/56543003/260001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/56543003/260001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/56543003/260001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/56543003/260001
Message was sent while issue was closed.
Change committed as 233442 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
