Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10790040:
JSON Schema Compiler now supports conversion from Choice to base::Value. (Closed)
|
Created:
8 years, 5 months ago by chebert Modified:
8 years, 5 months ago Reviewers:
not at google - send to devlin Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionJSON Schema Compiler now supports conversion from Choice to base::Value.
BUG=133636
TEST=choices_unittest.cc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147904
Patch Set 1 #Patch Set 2 : #Patch Set 3 : added comments/style #
Total comments: 39
Patch Set 4 : #
Total comments: 20
Patch Set 5 : Trying a refactor of GetChoiceValue and CreateEnumValue #
Total comments: 3
Patch Set 6 : GetChoiceType() no longer takes a parameter, and switch has a space after it #
Messages
Total messages: 11 (0 generated)
I'm not sure how you will feel about the from_function, but it was all I was able to think of.
nice one https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/cc_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:258: elif not prop.type_ == PropertyType.CHOICES: Prefer to structure this like if prop.type_ == PropertyType.ENUM: .. elif prop.type_ == PropertyType.CHOICES: # Ignore choices because...? pass else: ... Easier to casually read. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:279: function.params, from_function=True)) Like I said in the header, use from_client. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:296: def _GenerateGetChoiceValue(self, cpp_namespace, prop): nit: define below the place that it's used. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:297: """Generates GetChoiceValue() that returns a |scoped_ptr<base::Value>| usually |foo| is used for variables/parameters, not types. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:309: .Eblock('}') The {} in each of these case statements is unnecessary; they're all just "return blah". https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:314: .Append('return scoped_ptr<base::Value>(%s);' % can do "make_scoped_ptr(%s)" here https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:320: .Eblock('}') also add a blank line https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/h_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:276: If from_function is True if these properties belong to an API Function. You'll want to use from_client not from_function here, a property of the model (i.e. props). That's what determines whether ToValue is generated. (from_client means that the data originates on the client; meaning that we'll want to ToValue it). https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:293: if prop.from_json and not from_function: I think this should all be "prop.from_client"? https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:294: c.Append('scoped_ptr<base::Value> GetChoiceValue(%s) const;' % Btw this should be generated under private; should be easy enough to add an entirely different function that does that? _GeneratePrivatePropertyStructures... so should CreateEnumValue actually (possible cleanup) https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:296: self._cpp_type_generator.GetChoicesEnumType(prop))) There is some unfortunate whitespace thing going on in the generated code, namely: enum IntegersType { INTEGERS_NONE, INTEGERS_INTEGER, INTEGERS_ARRAY, }; scoped_ptr<base::Value> GetChoiceValue(IntegersType integers) const; enum StringsType { STRINGS_ARRAY, STRINGS_STRING, }; scoped_ptr<base::Value> GetChoiceValue(StringsType strings) const; IntegersType integers_type; (check out previewserver.py) The private thing might fix it. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/test/choices_unittest.cc (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:116: ListValue* strings_value(new ListValue()); nit: = new ListValue() https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:121: scoped_ptr<DictionaryValue> value(new DictionaryValue()); value on stack https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:122: value->Set("integers", integers_value); just inline integers_value here. Should be able to SetInteger("integers", 5) https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:125: scoped_ptr<ChoiceType> out(new ChoiceType); Just put on stack, use &out rather than out.get() etc. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:128: EXPECT_EQ(ChoiceType::STRINGS_ARRAY, out->strings_type); check values are correct too? https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:138: scoped_ptr<DictionaryValue> value(new DictionaryValue()); ditto value on stack https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:139: value->Set("integers", integers_value); ditto inline integers_value https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:142: scoped_ptr<ChoiceType> out(new ChoiceType); ditto ChoiceType()
https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/cc_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:258: elif not prop.type_ == PropertyType.CHOICES: On 2012/07/18 01:21:35, kalman wrote: > Prefer to structure this like > > if prop.type_ == PropertyType.ENUM: > .. > elif prop.type_ == PropertyType.CHOICES: > # Ignore choices because...? > pass > else: > ... > > Easier to casually read. Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:279: function.params, from_function=True)) On 2012/07/18 01:21:35, kalman wrote: > Like I said in the header, use from_client. Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:296: def _GenerateGetChoiceValue(self, cpp_namespace, prop): On 2012/07/18 01:21:35, kalman wrote: > nit: define below the place that it's used. Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:297: """Generates GetChoiceValue() that returns a |scoped_ptr<base::Value>| On 2012/07/18 01:21:35, kalman wrote: > usually |foo| is used for variables/parameters, not types. Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:309: .Eblock('}') On 2012/07/18 01:21:35, kalman wrote: > The {} in each of these case statements is unnecessary; they're all just "return > blah". Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:314: .Append('return scoped_ptr<base::Value>(%s);' % On 2012/07/18 01:21:35, kalman wrote: > can do "make_scoped_ptr(%s)" here Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:320: .Eblock('}') On 2012/07/18 01:21:35, kalman wrote: > also add a blank line Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/h_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:276: If from_function is True if these properties belong to an API Function. On 2012/07/18 01:21:35, kalman wrote: > You'll want to use from_client not from_function here, a property of the model > (i.e. props). That's what determines whether ToValue is generated. > > (from_client means that the data originates on the client; meaning that we'll > want to ToValue it). Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:293: if prop.from_json and not from_function: On 2012/07/18 01:21:35, kalman wrote: > I think this should all be "prop.from_client"? Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:294: c.Append('scoped_ptr<base::Value> GetChoiceValue(%s) const;' % Turns out the CreateEnumValue move to private is not quick and easy like the GetChoiceValue, so I will defer it to a future CL, in keeping with the small-CL mantra. On 2012/07/18 01:21:35, kalman wrote: > Btw this should be generated under private; should be easy enough to add an > entirely different function that does that? > > _GeneratePrivatePropertyStructures... > > so should CreateEnumValue actually (possible cleanup) https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:296: self._cpp_type_generator.GetChoicesEnumType(prop))) On 2012/07/18 01:21:35, kalman wrote: > There is some unfortunate whitespace thing going on in the generated code, > namely: > > enum IntegersType { > INTEGERS_NONE, > INTEGERS_INTEGER, > INTEGERS_ARRAY, > }; > > > scoped_ptr<base::Value> GetChoiceValue(IntegersType integers) const; > enum StringsType { > STRINGS_ARRAY, > STRINGS_STRING, > }; > > > scoped_ptr<base::Value> GetChoiceValue(StringsType strings) const; > IntegersType integers_type; > > (check out previewserver.py) > > The private thing might fix it. Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/test/choices_unittest.cc (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:116: ListValue* strings_value(new ListValue()); On 2012/07/18 01:21:35, kalman wrote: > nit: = new ListValue() Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:121: scoped_ptr<DictionaryValue> value(new DictionaryValue()); On 2012/07/18 01:21:35, kalman wrote: > value on stack Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:122: value->Set("integers", integers_value); On 2012/07/18 01:21:35, kalman wrote: > just inline integers_value here. Should be able to SetInteger("integers", 5) Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:125: scoped_ptr<ChoiceType> out(new ChoiceType); On 2012/07/18 01:21:35, kalman wrote: > Just put on stack, use &out rather than out.get() etc. Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:128: EXPECT_EQ(ChoiceType::STRINGS_ARRAY, out->strings_type); On 2012/07/18 01:21:35, kalman wrote: > check values are correct too? Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:138: scoped_ptr<DictionaryValue> value(new DictionaryValue()); On 2012/07/18 01:21:35, kalman wrote: > ditto value on stack Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:139: value->Set("integers", integers_value); On 2012/07/18 01:21:35, kalman wrote: > ditto inline integers_value Done. https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/test/choices_unittest.cc:142: scoped_ptr<ChoiceType> out(new ChoiceType); On 2012/07/18 01:21:35, kalman wrote: > ditto ChoiceType() Done.
https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... File tools/json_schema_compiler/h_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/3001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:294: c.Append('scoped_ptr<base::Value> GetChoiceValue(%s) const;' % On 2012/07/19 00:56:01, chebert wrote: > Turns out the CreateEnumValue move to private is not quick and easy like the > GetChoiceValue, so I will defer it to a future CL, in keeping with the small-CL > mantra. > > On 2012/07/18 01:21:35, kalman wrote: > > Btw this should be generated under private; should be easy enough to add an > > entirely different function that does that? > > > > _GeneratePrivatePropertyStructures... > > > > so should CreateEnumValue actually (possible cleanup) > sure thing https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... File tools/json_schema_compiler/cc_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:257: self._cpp_type_generator.GetEnumNoneValue(prop))) nit: these should vertically line up, so unindent this 4 characters https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:261: self._cpp_type_generator.GetEnumNoneValue(prop))) ditto oh it was already like this. incremental cleanup! https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:582: If from_function is True if these properties belong to an API Function. revert this comment change https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:597: if param.from_client and param.from_json: why does it need to be both from the JSON? i would have though the correct condition was just "param.from_client"? https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:604: def _GenerateGetChoiceValue(self, cpp_namespace, prop): this code is so similar to _GenerateCreateEnumValue. However, it may be a bit of a contrivance to try and share code between the two. Oh well. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:609: (c.Sblock('scoped_ptr<base::Value> ' static? (const would also be unnecessary) https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:619: (c.Append('default:') we are generating a branch for every enum value, so it shouldn't be possible to hit this unless there's a bug somewhere (which involves casting a non-enum value to one of these enums; and *that* shouldn't be possible because the only thing calling this code is our own code). So rather than switch (...) { case FOO: return ...; case BAR: return ...; default: return scoped_ptr<base::Value>(); } let's generate switch (...) { case FOO: return ...; case BAR: return ...; } NOTREACHED(); return scoped_ptr<base::Value>(); (make sure this file #includes base/logging.h) that would also require generating an explicit case for "none" if it's optional... https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:656: .Append('}') ditto default thing. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... File tools/json_schema_compiler/h_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:320: if prop.from_json and prop.from_client: ditto, why prop.from_json? https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:321: c.Append('scoped_ptr<base::Value> GetChoiceValue(%s) const;' % static?
https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... File tools/json_schema_compiler/cc_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:257: self._cpp_type_generator.GetEnumNoneValue(prop))) On 2012/07/19 01:48:22, kalman wrote: > nit: these should vertically line up, so unindent this 4 characters Done. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:261: self._cpp_type_generator.GetEnumNoneValue(prop))) On 2012/07/19 01:48:22, kalman wrote: > ditto > > oh it was already like this. incremental cleanup! Done. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:582: If from_function is True if these properties belong to an API Function. On 2012/07/19 01:48:22, kalman wrote: > revert this comment change Done. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:597: if param.from_client and param.from_json: On 2012/07/19 01:48:22, kalman wrote: > why does it need to be both from the JSON? i would have though the correct > condition was just "param.from_client"? Done. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:604: def _GenerateGetChoiceValue(self, cpp_namespace, prop): I took a shot at this. It is a bit contrived though... On 2012/07/19 01:48:22, kalman wrote: > this code is so similar to _GenerateCreateEnumValue. However, it may be a bit of > a contrivance to try and share code between the two. Oh well. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:609: (c.Sblock('scoped_ptr<base::Value> ' non-static, because we return member data. const, because the "this" reference is const (from the ToValue() method). On 2012/07/19 01:48:22, kalman wrote: > static? (const would also be unnecessary) https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:619: (c.Append('default:') On 2012/07/19 01:48:22, kalman wrote: > we are generating a branch for every enum value, so it shouldn't be possible to > hit this unless there's a bug somewhere (which involves casting a non-enum value > to one of these enums; and *that* shouldn't be possible because the only thing > calling this code is our own code). > > So rather than > > switch (...) { > case FOO: > return ...; > case BAR: > return ...; > default: > return scoped_ptr<base::Value>(); > } > > let's generate > > switch (...) { > case FOO: > return ...; > case BAR: > return ...; > } > > NOTREACHED(); > return scoped_ptr<base::Value>(); > > (make sure this file #includes base/logging.h) > > that would also require generating an explicit case for "none" if it's > optional... Done. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:656: .Append('}') On 2012/07/19 01:48:22, kalman wrote: > ditto default thing. Done. https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... File tools/json_schema_compiler/h_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:320: if prop.from_json and prop.from_client: see cc_generator comment. On 2012/07/19 01:48:22, kalman wrote: > ditto, why prop.from_json? https://chromiumcodereview.appspot.com/10790040/diff/8001/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:321: c.Append('scoped_ptr<base::Value> GetChoiceValue(%s) const;' % see cc_generator comment. On 2012/07/19 01:48:22, kalman wrote: > static?
lgtm https://chromiumcodereview.appspot.com/10790040/diff/5003/tools/json_schema_c... File tools/json_schema_compiler/cc_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/5003/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:608: (c.Sblock('scoped_ptr<base::Value> ' where's the static? Should be able to be static like CreateEnumValue. https://chromiumcodereview.appspot.com/10790040/diff/5003/tools/json_schema_c... tools/json_schema_compiler/cc_generator.py:665: """Generates a single return case for a switch block. haha, sure that's cool. https://chromiumcodereview.appspot.com/10790040/diff/5003/tools/json_schema_c... File tools/json_schema_compiler/h_generator.py (right): https://chromiumcodereview.appspot.com/10790040/diff/5003/tools/json_schema_c... tools/json_schema_compiler/h_generator.py:322: c.Append('scoped_ptr<base::Value> GetChoiceValue(%s) const;' % static?
Sorry, I read that wrong. I didn't realise that these were generating instance
methods, since they all have the same name.
2 things I noticed in the generated code:
1. I'm seeing switch(strings) but it should be switch (strings)
2. I'm also seeing "this->" but that should be unnecessary.
3. The bigger one:
Code looks like this:
scoped_ptr<base::DictionaryValue> ChoiceType::ToValue() {
value->SetWithoutPathExpansion("integers",
GetChoiceValue(this->integers_type).release());
...
}
scoped_ptr<base::Value> ChoiceType::GetChoiceValue(IntegersType integers) const
{
switch(integers) {
case INTEGERS_ARRAY:
return
make_scoped_ptr<base::Value>(json_schema_compiler::util::CreateValueFromOptionalArray(integers_array).release());
case INTEGERS_INTEGER:
return
make_scoped_ptr<base::Value>(base::Value::CreateIntegerValue(*integers_integer));
}
NOTREACHED();
return scoped_ptr<base::Value>();
}
Which is an odd mix of static and instance information for GetChoiceValue. It
shouldn't need to be passed in the "integers" type, because it can read it
itself:
scoped_ptr<base::DictionaryValue> ChoiceType::ToValue() {
value->SetWithoutPathExpansion("integers", GetChoiceValue().release());
...
}
scoped_ptr<base::Value> ChoiceType::GetChoiceValue() const {
switch(integers_type) {
case INTEGERS_ARRAY:
return
make_scoped_ptr<base::Value>(json_schema_compiler::util::CreateValueFromOptionalArray(integers_array).release());
case INTEGERS_INTEGER:
return
make_scoped_ptr<base::Value>(base::Value::CreateIntegerValue(*integers_integer));
}
NOTREACHED();
return scoped_ptr<base::Value>();
}
however, this won't actually work because there will be multiple
"GetChoiceValue" functions and the compiler wouldn't know which one to pick. So
I think you'll need to include the name of the type in the method, e.g.
GetIntegersChoiceValue.
> 1. I'm seeing switch(strings) but it should be switch (strings)
Done.
> 2. I'm also seeing "this->" but that should be unnecessary.
This is only necessary because of naming conflicts. (since there is a "value"
local variable and a "value" instance variable. I think leaving it is safer and
easier than trying to remove naming conflicts. (And it was also like this
before.)
> 3. The bigger one:
> Code looks like this:
> scoped_ptr<base::DictionaryValue> ChoiceType::ToValue() {
> value->SetWithoutPathExpansion("integers",
> GetChoiceValue(this->integers_type).release());
> ...
> }
>
> scoped_ptr<base::Value> ChoiceType::GetChoiceValue(IntegersType integers)
const
> {
> switch(integers) {
> case INTEGERS_ARRAY:
> return
>
make_scoped_ptr<base::Value>(json_schema_compiler::util::CreateValueFromOptionalArray(integers_array).release());
> case INTEGERS_INTEGER:
> return
>
make_scoped_ptr<base::Value>(base::Value::CreateIntegerValue(*integers_integer));
> }
> NOTREACHED();
> return scoped_ptr<base::Value>();
> }
>
> Which is an odd mix of static and instance information for GetChoiceValue. It
> shouldn't need to be passed in the "integers" type, because it can read it
> itself:
>
> scoped_ptr<base::DictionaryValue> ChoiceType::ToValue() {
> value->SetWithoutPathExpansion("integers", GetChoiceValue().release());
> ...
> }
>
> scoped_ptr<base::Value> ChoiceType::GetChoiceValue() const {
> switch(integers_type) {
> case INTEGERS_ARRAY:
> return
>
make_scoped_ptr<base::Value>(json_schema_compiler::util::CreateValueFromOptionalArray(integers_array).release());
> case INTEGERS_INTEGER:
> return
>
make_scoped_ptr<base::Value>(base::Value::CreateIntegerValue(*integers_integer));
> }
> NOTREACHED();
> return scoped_ptr<base::Value>();
> }
>
> however, this won't actually work because there will be multiple
> "GetChoiceValue" functions and the compiler wouldn't know which one to pick.
So
> I think you'll need to include the name of the type in the method, e.g.
> GetIntegersChoiceValue.
Done.
nice, lgtm (again)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10790...
Change committed as 147904 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
