Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
8 years, 10 months ago by calamity Modified:
8 years, 10 months ago Reviewers:
Yoyo Zhou, battre, not at google - send to devlin CC:
chromium-reviews, Aaron Boodman, pam+watch_chromium.org, mihaip+watch_chromium.org, battre, koz (OOO until 15th September), benwells Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdds support for the "choices" and "any" types to json_schema_compiler, as well
as miscellaneous improvements to get at least 3 more schema files compiling:
windows, tabs, and the in-progress experimental.declarative.
For description of the generator, see http://codereview.chromium.org/9114036/
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122082
Patch Set 1 #Patch Set 2 : DictionaryValue* to linked_ptr<DictionaryValue> #
Total comments: 6
Patch Set 3 : fixed optional properties #
Total comments: 94
Patch Set 4 : a few comments, fixes #
Total comments: 16
Patch Set 5 : rework #
Total comments: 20
Patch Set 6 : support for choices #
Total comments: 52
Patch Set 7 : more rework #
Total comments: 41
Patch Set 8 : rework, updated base #
Total comments: 30
Patch Set 9 : rework #Patch Set 10 : remove generated files, move gypi into api.gyp to make windows happy #Patch Set 11 : fix win build for real this time? #Patch Set 12 : rework, add a couple of tests #
Created: 8 years, 10 months ago
Messages
Total messages: 21 (0 generated)
Only started looking at this, but noticed that. I'll keep looking at it now, but I thought you might want to fix that up now. // (Removed CC's since this isn't really a review yet): chromium-reviews@chromium.org, Aaron Boodman, pam+watch@chromium.org, mihaip+watch@chromium.org http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... File generated_files_will_not_submit/experimental.declarative.cc (right): http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:30: dict->GetInteger("priority", out->priority.get()); I don't think this is right. priority is optional, so when it's not set it needs to be NULL. However, as the code currently is, if not set it's given whatever value "new int()" is initialised to. Possibly 0? In any case, an actual pointer to some memory, not NULL. This needs to be more like: { int priority_temp; if (dict->GetInteger("priority", &priority_tmp)) out->priority.reset(new int(priority_tmp)); } http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:43: value->SetWithoutPathExpansion("priority", Value::CreateIntegerValue(*priority)); Ditto, priority is optional. Maybe the optional/non-optional logic is messed up? Anyway, this needs to be if (priority.get()) ... http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:45: value->SetWithoutPathExpansion("id", Value::CreateStringValue(*id)); ditto
http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... File generated_files_will_not_submit/experimental.declarative.cc (right): http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:30: dict->GetInteger("priority", out->priority.get()); On 2012/02/02 03:37:43, kalman wrote: > I don't think this is right. priority is optional, so when it's not set it needs > to be NULL. However, as the code currently is, if not set it's given whatever > value "new int()" is initialised to. Possibly 0? In any case, an actual pointer > to some memory, not NULL. > > This needs to be more like: > > { > int priority_temp; > if (dict->GetInteger("priority", &priority_tmp)) > out->priority.reset(new int(priority_tmp)); > } Done. http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:43: value->SetWithoutPathExpansion("priority", Value::CreateIntegerValue(*priority)); On 2012/02/02 03:37:43, kalman wrote: > Ditto, priority is optional. Maybe the optional/non-optional logic is messed up? > > Anyway, this needs to be > if (priority.get()) > ... Done. http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:45: value->SetWithoutPathExpansion("id", Value::CreateStringValue(*id)); On 2012/02/02 03:37:43, kalman wrote: > ditto Done.
Awesome. I haven't really gone through the real gritty code generation stuff in the cc/h/cpp.py classes but I will soon. I think my round of style-picking is mostly over though, and that's enough to go on for now... General note: there seems to be a lot more code in the generator, but not a whole lot more in the way of tests. http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... File chrome/common/extensions/api/api.gyp (right): http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... chrome/common/extensions/api/api.gyp:10: 'experimental.declarative.json', you'll need to delete this before submitting http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... File chrome/common/extensions/api/cookies.json (right): http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... chrome/common/extensions/api/cookies.json:4: "compile": true, better be prepared to follow through with that :) (or just revert for now) actually, that makes me think, do we still need to set this property? the way the build is set up at the moment is to list the appropriate scheme files in the gyp; is that enough to know to compile it now? (and then have a nocompile like there is a nodoc... I know you already had it like this, but it seems more sensible, unless I'm missing something) http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... File chrome/common/extensions/api/experimental.declarative.json (right): http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... chrome/common/extensions/api/experimental.declarative.json:1: [ you'll need to delete this before submitting http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... File generated_files_will_not_submit/experimental.declarative.cc (right): http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:30: if(dict->GetInteger("priority", &priority_temp)) space between "if" and the ( The reason why I had priority_temp inside { int priority_temp; ... } is to avoid any naming conflicts, which could happen if some schema has both a "foo" and a "fooTemp" property defined. Highly unlikely, but if it's a cheap thing to avoid then we might as well. Besides which, I personally just like the idiom. http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:32: if(!json_schema_compiler::util::GetArrayFromDictionary(*dict, "conditions", &out->conditions)) ditto with the "if" (etc) http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:45: if(priority.get()) and here http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:65: extra \n http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... File generated_files_will_not_submit/permissions.h (right): http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/permissions.h:8: is there any difference between how this file and permissions.cc used to be generated? it's a pretty hard thing to determine, I suppose. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:94: .Sblock('bool %(namespace)s::Populate(const Value& value, %(name)s* out) {') line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:117: elif prop.type_ == PropertyType.REF or prop.type_ == PropertyType.OBJECT: line wrap here and above http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:123: .Append(' out->%(name)s.reset(new %(type)s(%(name)s_temp));') yeah, needs space after if. same below. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:182: param_namespace = '%s::Params::%s' % (classname, cpp_util.Classname(param.name)) line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:208: #.Append(' return scoped_ptr<Params>();') explain why this is commented out; or just delete the commented-out lines. seeing code that's commented out without explaining why can be pretty confusing. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:238: if param.type_ == PropertyType.REF or param.type_ == PropertyType.OBJECT: line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:252: (c.Append('if(!%s)' % cpp_util.GetValueFromList(param, 'args', i, dst)) line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:291: .Append('%s;' % self._util_cc_helper.SetArrayToList(param, param.unix_name, 'value')) line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:295: c.Append('return %s;' % cpp_util.CreateValueFromSingleProperty(param, param.unix_name)) line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:56: self.GetCppNamespaceName(self._namespace)) weird indentation (and below) http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:128: for dependency_namespace in sorted(self._NamespaceTypeDependencies().keys()): line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_util.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_util.py:64: var: raw value "raw value"? http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_util.py:71: raise NotImplementedError('Conversion of single %s to Value not implemented' % line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:48: forward_declarations = self._cpp_type_generator.GenerateForwardDeclarations(); line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:101: (self._cpp_type_generator.GetType(prop, wrap_optional=True), prop.unix_name)) line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:153: line wrap http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:62: if param.get('choices'): I think you need to document what the available properties are throughout the model, it's a bit hard to follow just by looking through the code. I don't know what the standard python documentation structure looks like, but something like (at the top) """Model of all namespaces that comprise an API. Properties: - |namespaces| a map of namespace to its |Namespace| model """ For namespace """An API namespace Properties: - |name| foo - |source_file| bar """ And, the reason I mention this, """A callback parameter to a Function. Properties: - |params| what is params, why does each choice result in something being appended to it? """ http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:85: self.params.append(_Choice(self.name, choice)) (ditto) I see now why you are appending a different Choice for each choice, but this definitely needs some extra explanation. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:97: def __init__(self, name, json, is_choice=False): is_choice no longer needed http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:98: self.name = name I think for self-documentation sake, this should specify what the name is expecting to be. which is... lowerCamelCase I think, right? something like if not re.match('^[a-z][a-zA-Z0-9]*$', name) throw AppropriateException(name + " must be lowerCamelCase") http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:109: elif json_type == 'any': extra space http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:115: elif json_type == 'double' or json_type == 'number': We should remove support for "double" and fix up the JSON in the 1 place that it's used. (though might as well leave PropertyType as DOUBLE) http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:126: self.properties[key] = _Choice(self.name, choice) doesn't this overwrite the same (and incorrect) key each time? should be something like if 'choices' in val: for choice in val['choices']: choice_property = _Choice(self.name, choice) self.properties[choice_property.name_] = choice_property http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:155: def _UnixName(name): document http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:157: for x in re.findall('[A-Z][a-z_]*', name[0].upper() + name[1:])]) i'll just assume that works... http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:159: def _Choice(name, json): document http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:162: prop_type = json['$ref'].lower() I think should structure this slightly differently to make the intent a bit clearer (and so avoid having to mutate the property that is generated). def _Choice(name, json): """documentation. """ # Adjust name so that it reflects the type of the choice. # E.g. for a property with name "ids" with choices ["integer", "array"], # we want to generate properties "ids_integer" and "ids_array". property_type = json.get('type') if not property_type: property_type = json['$ref'] if not property_type: throw NotImplementedError(json) full_name = "%s%s" % (name, propertyType[0].upper() + propertyType[1].lower()) return Property(full_name, json) http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... File tools/json_schema_compiler/util.cc (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.cc:28: bool GetItemFromList(const ListValue& from, int index, linked_ptr<base::DictionaryValue>* out) { fix line length http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.cc:49: void AddItemToList(const linked_ptr<base::DictionaryValue> from, base::ListValue* out) { line length http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... File tools/json_schema_compiler/util.h (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:13: #include "base/memory/linked_ptr.h" :sort http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:19: // Creates a new item at |out| from |from|[|index|]. Explain that these are template specializations for use with the Get(Optional)ArrayFromDictionary methods. And group them in there (i.e. group the "getters" and "setters" together) I'd put it under the actual template declaration for this (i.e. after line 51) http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:33: if(!T::Populate(*dict, obj)) space after if http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:40: void AddItemToList(const int from, base::ListValue* out); Ditto explanation/ordering. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:43: // TODO(calamity) does std::string need &? yes http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:56: base::ListValue* out) { Put on same line as above http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:66: base::ListValue* out) { ditto http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:69: SetArrayToList(from, out); heh, a little more concise as if (from.get()) SetArrayToList(from, out); ditto below http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:85: // GetItemFromList to be implemented for |T|. run gq over this http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:97: // Creates a new vector containing the fundamentals |list| at |out|. Returns what does "fundamentals" mean? http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:116: // other than |T|. gq here too http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... File tools/json_schema_compiler/util_cc_helper.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util_cc_helper.py:8: """ line length http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util_cc_helper.py:10: self.type_manager = type_manager type_manager should be private http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util_cc_helper.py:71: val = '%(namespace)s::SetOptionalArrayToDictionary(%(src)s, "%(name)s", %(dst)s)' line wrap
a couple of other comments. I'll hold off having another look until you've gone through the last lot and these. ping me when you have. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:281: for param in params: so this generates a different Create method for each parameter? you'd better explain that this only happens when there are choices, since otherwise that would be surprising. (and that there's only either 0 or 1 parameter, which is why this works) http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:288: arg = 'const %(type)s& %(name)s' bit odd seeing this. how about if param.type == PropertyType.REF: arg = 'const %(type)s& %(name)s' else arg = 'const %(type)s %(name)s' http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:289: arg = arg % sub looks like python has an %= operator, you should use that; and inline sub, since it's only used once before being overridden. arg %= { 'type': ... 'name': ... } I mean. But that was probably obvious. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:303: c.Substitute(sub) ditto, inline sub http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:97: cpp_type = 'linked_ptr<DictionaryValue> ' Why linked_ptr<> over a raw type? I presume it's because they're the most versatile way of having pointers, but they're only needed inside stl containers. Hopefully when they're by themselves ("single value" or something you're calling them?) they can be raw types. If possible and easy, try doing that; only wrap in linked_ptr if it's an array, otherwise make raw. As discussed, we can't find any supported schemas which would actually test this; so cross this bridge when we come to it. Or better, add an appropriate NotImplemented exception in the right place. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:114: return '%s' % cpp_type um, "return cpp_type"; ("%s" % cpp_type) is a bit redundant. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:183: for param in params: same; explain why generating a new Create() function for each parameter makes sense. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:191: arg = arg % sub same deal, arg %= { 'type': ... 'name': ... }
http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... File chrome/common/extensions/api/cookies.json (right): http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api... chrome/common/extensions/api/cookies.json:4: "compile": true, On 2012/02/05 23:42:12, kalman wrote: > better be prepared to follow through with that :) > > (or just revert for now) > > actually, that makes me think, do we still need to set this property? the way > the build is set up at the moment is to list the appropriate scheme files in the > gyp; is that enough to know to compile it now? > > (and then have a nocompile like there is a nodoc... I know you already had it > like this, but it seems more sensible, unless I'm missing something) It's enough for now, but there's no facility for someone that wants to include non-compiled types in the model. I'll think about that but for now, removed. http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... File generated_files_will_not_submit/experimental.declarative.cc (right): http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:30: if(dict->GetInteger("priority", &priority_temp)) On 2012/02/05 23:42:12, kalman wrote: > space between "if" and the ( > > The reason why I had priority_temp inside > { > int priority_temp; > ... > } > > is to avoid any naming conflicts, which could happen if some schema has both a > "foo" and a "fooTemp" property defined. Highly unlikely, but if it's a cheap > thing to avoid then we might as well. > > Besides which, I personally just like the idiom. Done. http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:32: if(!json_schema_compiler::util::GetArrayFromDictionary(*dict, "conditions", &out->conditions)) On 2012/02/05 23:42:12, kalman wrote: > ditto with the "if" (etc) Done. http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:45: if(priority.get()) On 2012/02/05 23:42:12, kalman wrote: > and here Done. http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/experimental.declarative.cc:65: On 2012/02/05 23:42:12, kalman wrote: > extra \n Done. http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... File generated_files_will_not_submit/permissions.h (right): http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/permissions.h:8: On 2012/02/05 23:42:12, kalman wrote: > is there any difference between how this file and permissions.cc used to be > generated? > > it's a pretty hard thing to determine, I suppose. Not really? I just included it for sake of completeness (and laziness). http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:94: .Sblock('bool %(namespace)s::Populate(const Value& value, %(name)s* out) {') On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:208: #.Append(' return scoped_ptr<Params>();') On 2012/02/05 23:42:12, kalman wrote: > explain why this is commented out; or just delete the commented-out lines. > > seeing code that's commented out without explaining why can be pretty confusing. Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:238: if param.type_ == PropertyType.REF or param.type_ == PropertyType.OBJECT: On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:252: (c.Append('if(!%s)' % cpp_util.GetValueFromList(param, 'args', i, dst)) On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:291: .Append('%s;' % self._util_cc_helper.SetArrayToList(param, param.unix_name, 'value')) On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:295: c.Append('return %s;' % cpp_util.CreateValueFromSingleProperty(param, param.unix_name)) On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:56: self.GetCppNamespaceName(self._namespace)) On 2012/02/05 23:42:12, kalman wrote: > weird indentation (and below) Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:128: for dependency_namespace in sorted(self._NamespaceTypeDependencies().keys()): On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_util.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_util.py:71: raise NotImplementedError('Conversion of single %s to Value not implemented' % On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:48: forward_declarations = self._cpp_type_generator.GenerateForwardDeclarations(); On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:101: (self._cpp_type_generator.GetType(prop, wrap_optional=True), prop.unix_name)) On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:153: On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:62: if param.get('choices'): On 2012/02/05 23:42:12, kalman wrote: > I think you need to document what the available properties are throughout the > model, it's a bit hard to follow just by looking through the code. > > I don't know what the standard python documentation structure looks like, but > something like (at the top) > > """Model of all namespaces that comprise an API. > > Properties: > - |namespaces| a map of namespace to its |Namespace| model > """ > > For namespace > > """An API namespace > > Properties: > - |name| foo > - |source_file| bar > """ > > And, the reason I mention this, > > """A callback parameter to a Function. > > Properties: > - |params| what is params, why does each choice result in something being > appended to it? > """ Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:85: self.params.append(_Choice(self.name, choice)) On 2012/02/05 23:42:12, kalman wrote: > (ditto) > > I see now why you are appending a different Choice for each choice, but this > definitely needs some extra explanation. Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:97: def __init__(self, name, json, is_choice=False): On 2012/02/05 23:42:12, kalman wrote: > is_choice no longer needed I think I'll need something eventually in order to generate the type indicator for which choice is populated. The choices code is certainly not finished. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:98: self.name = name On 2012/02/05 23:42:12, kalman wrote: > I think for self-documentation sake, this should specify what the name is > expecting to be. which is... lowerCamelCase I think, right? > > something like > > if not re.match('^[a-z][a-zA-Z0-9]*$', name) > throw AppropriateException(name + " must be lowerCamelCase") Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:109: elif json_type == 'any': On 2012/02/05 23:42:12, kalman wrote: > extra space Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:115: elif json_type == 'double' or json_type == 'number': On 2012/02/05 23:42:12, kalman wrote: > We should remove support for "double" and fix up the JSON in the 1 place that > it's used. > > (though might as well leave PropertyType as DOUBLE) Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:126: self.properties[key] = _Choice(self.name, choice) On 2012/02/05 23:42:12, kalman wrote: > doesn't this overwrite the same (and incorrect) key each time? > > should be something like > > if 'choices' in val: > for choice in val['choices']: > choice_property = _Choice(self.name, choice) > self.properties[choice_property.name_] = choice_property Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:155: def _UnixName(name): On 2012/02/05 23:42:12, kalman wrote: > document Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:159: def _Choice(name, json): On 2012/02/05 23:42:12, kalman wrote: > document Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... File tools/json_schema_compiler/util.cc (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.cc:28: bool GetItemFromList(const ListValue& from, int index, linked_ptr<base::DictionaryValue>* out) { On 2012/02/05 23:42:12, kalman wrote: > fix line length Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... File tools/json_schema_compiler/util.h (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:13: #include "base/memory/linked_ptr.h" On 2012/02/05 23:42:12, kalman wrote: > :sort Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:19: // Creates a new item at |out| from |from|[|index|]. On 2012/02/05 23:42:12, kalman wrote: > Explain that these are template specializations for use with the > Get(Optional)ArrayFromDictionary methods. > > And group them in there (i.e. group the "getters" and "setters" together) > > I'd put it under the actual template declaration for this (i.e. after line 51) Done. Couldn't move these under the templates that use them because the templates needed to know the functions existed. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:33: if(!T::Populate(*dict, obj)) On 2012/02/05 23:42:12, kalman wrote: > space after if Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:43: // TODO(calamity) does std::string need &? On 2012/02/05 23:42:12, kalman wrote: > yes Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:56: base::ListValue* out) { On 2012/02/05 23:42:12, kalman wrote: > Put on same line as above Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:66: base::ListValue* out) { On 2012/02/05 23:42:12, kalman wrote: > ditto Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:69: SetArrayToList(from, out); On 2012/02/05 23:42:12, kalman wrote: > heh, a little more concise as > > if (from.get()) > SetArrayToList(from, out); > > ditto below Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:85: // GetItemFromList to be implemented for |T|. On 2012/02/05 23:42:12, kalman wrote: > run gq over this Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:97: // Creates a new vector containing the fundamentals |list| at |out|. Returns On 2012/02/05 23:42:12, kalman wrote: > what does "fundamentals" mean? Sorry, forgot to change this doc. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util.h:116: // other than |T|. On 2012/02/05 23:42:12, kalman wrote: > gq here too Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... File tools/json_schema_compiler/util_cc_helper.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util_cc_helper.py:8: """ On 2012/02/05 23:42:12, kalman wrote: > line length Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util_cc_helper.py:10: self.type_manager = type_manager On 2012/02/05 23:42:12, kalman wrote: > type_manager should be private Done. http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u... tools/json_schema_compiler/util_cc_helper.py:71: val = '%(namespace)s::SetOptionalArrayToDictionary(%(src)s, "%(name)s", %(dst)s)' On 2012/02/05 23:42:12, kalman wrote: > line wrap Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:281: for param in params: On 2012/02/06 04:19:17, kalman wrote: > so this generates a different Create method for each parameter? > > you'd better explain that this only happens when there are choices, since > otherwise that would be surprising. > > (and that there's only either 0 or 1 parameter, which is why this works) Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:288: arg = 'const %(type)s& %(name)s' On 2012/02/06 04:19:17, kalman wrote: > bit odd seeing this. > > how about > > if param.type == PropertyType.REF: > arg = 'const %(type)s& %(name)s' > else > arg = 'const %(type)s %(name)s' Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:289: arg = arg % sub On 2012/02/06 04:19:17, kalman wrote: > looks like python has an %= operator, you should use that; and inline sub, since > it's only used once before being overridden. > > arg %= { > 'type': ... > 'name': ... > } > > I mean. But that was probably obvious. Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:303: c.Substitute(sub) On 2012/02/06 04:19:17, kalman wrote: > ditto, inline sub Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:97: cpp_type = 'linked_ptr<DictionaryValue> ' On 2012/02/06 04:19:17, kalman wrote: > Why linked_ptr<> over a raw type? I presume it's because they're the most > versatile way of having pointers, but they're only needed inside stl containers. > Hopefully when they're by themselves ("single value" or something you're calling > them?) they can be raw types. > > If possible and easy, try doing that; only wrap in linked_ptr if it's an array, > otherwise make raw. > > As discussed, we can't find any supported schemas which would actually test > this; so cross this bridge when we come to it. Or better, add an appropriate > NotImplemented exception in the right place. Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:114: return '%s' % cpp_type On 2012/02/06 04:19:17, kalman wrote: > um, "return cpp_type"; ("%s" % cpp_type) is a bit redundant. Yes, yes it is. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:183: for param in params: On 2012/02/06 04:19:17, kalman wrote: > same; explain why generating a new Create() function for each parameter makes > sense. Done. http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h... tools/json_schema_compiler/h_generator.py:191: arg = arg % sub On 2012/02/06 04:19:17, kalman wrote: > same deal, > > arg %= { > 'type': ... > 'name': ... > } Done.
Looking really nice, adding back the CC list (plus koz/benwells) and reviewers (aa/yoz). Hi guys. Also CC'ed battre who's writing the declarative API. I hope this CL description/contents makes sense out of context? Basically we're working towards committing a version of the compiler that will work for Dominic as he's writing the declarative API (which is already in review, and doing this stuff by hand). Basically yeah: - my style eyes are happy, though they're not good at spotting pythons - structure is good, and your code seems really nice to iterate on - there is just that "choices" thing which might need a bit of a reshuffle (sorry about that again) - also, tests still seem meagre. I've observed you doing a lot of compiling and running browser tests to test this stuff; you're much better off writing comprehensive tests here and iterating that way, rather than on the browser. Plus, APIs that people write in the future won't have that luxury obviously. - also, run try jobs :) http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... File generated_files_will_not_submit/permissions.h (right): http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub... generated_files_will_not_submit/permissions.h:8: On 2012/02/06 11:51:18, calamity wrote: > On 2012/02/05 23:42:12, kalman wrote: > > is there any difference between how this file and permissions.cc used to be > > generated? > > > > it's a pretty hard thing to determine, I suppose. > > Not really? I just included it for sake of completeness (and laziness). (not quite what I meant but nvm, not important) http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:97: def __init__(self, name, json, is_choice=False): On 2012/02/06 11:51:18, calamity wrote: > On 2012/02/05 23:42:12, kalman wrote: > > is_choice no longer needed > > I think I'll need something eventually in order to generate the type indicator > for which choice is populated. The choices code is certainly not finished. Yeah, see the other couple of comments I've made regarding this elsewhere. http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:213: if last != param.name: Why the "last" check? Is it something to do with choices? If so, wouldn't this be a bit dangerous to assume they all have the wrong name (see other comment). Yes, must be choices. That's a bummer :( Hmm, a few options here I suppose - cache those values directly in the model (we can test it that way, but it makes the model a bit bigger and that's a shame) - tag the parameters in a way that can detect whether they're part of the same choices group (sounds arcane and complicated; too much logic in the C generators) - do choices differently; push that "expansion" logic out of the model and into the c generators. params just becomes literally the params (with a CHOICES option) and h_generator/cc_generator/cpp_types_generator know how to expand them. I reckon the last sounds the best. This method then becomes required = 0 for param in function.params: if not param.optional required += 1 c = Code() 'if %(var)s.GetSize() < %(required)d' ' return scoped_ptr<Params>();' 'if %(var)s.GetSize() > %(total)d' ' return scoped_ptr<Params>();' return c.sub({ 'var': var, 'required': required, 'total': params.length }) http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cc_generator.py:222: check = 'if (%(var)s.GetSize() < %(req)d && %(var)s.GetSize() > %(sum)d)' && should be || right? http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... File tools/json_schema_compiler/compiler.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/compiler.py:82: filename_suffix) Note here, and in general, indentation here makes it look like filename_suffix is an argument to AddNamespace(). The c++ style guide would say to format this call like type_generator.AddNamespace( referenced_namespace, cpp_util.ClassName(referenced_namespace.name).lower() + filename_suffix) and if that doesn't fit then type_generator.AddNamespace( referenced_namespace, cpp_util.ClassName(referenced_namespace.name).lower() + filename_suffix) i.e. all arguments/parameters should be on same line, or in line. That's open a bit to interpretation I suppose, but I think the same sentiment should translate across to Python with these things. ^ apply to all line-splitting choices. http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:98: if prop.item_type.type_ in (PropertyType.REF, PropertyType.ANY, PropertyType.OBJECT): Cool Line wrapping though :) http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:130: its model.Type line wrap http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:205: def _Choice(name, json): I made a comment here earlier about structuring this method differently, but it seems to have gotten lost. No matter, since I've come to realise something after looking at that checking code. Sorry to have not have formed an opinion about this earlier... you're going to hate me... but the way choices are done at the moment is hard to understand and easy to misuse. There is logic in in the model to expand choice parameters, but not enough (as we saw earlier) and it's not applicable to all scenarios anyway. To write this differently, how about: - Have another PropertyType CHOICE. - If a Property is CHOICE, have another member of Property called like "choices", an array of possible Properties. - Code that uses parameters needs to know how to expand out choices. A lot of this, I think, should be able to be abstracted nicely away into the CppTypeGenerator class; like a method along the lines of ExpandChoicesInParams or something which returns an array of Properties, like function.params, but expanded in the same way you're doing here, basically. (and then the h/cc generator classes in ToValue which used function.params and assumed the Choices were taken care of would use cpp_type_generator.ExpandChoicesInParams(function.params) instead) Yeah, there will be a few non-trivial changes to do something like this, but nothing overly structural, and I think overall it will result in easier to follow code. And... you'll need tests for it. http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:208: raise NotImplementedError('%s\nChoices is not fully implemented' % json) Choices is implemented for return values right? Or one of them? I just meant to raise a NotImplementedError where you said it didn't work, not for _all_ attempts to use choices :) That's a bit harsh on yourself...
http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m... tools/json_schema_compiler/model.py:205: def _Choice(name, json): > - If a Property is CHOICE, have another member of Property called like > "choices", an array of possible Properties. Sorry, I should have said an array of possible PropertyTypes. This is tough though, since for array/object you also need the information about the items/properties. Perhaps you'll need to eventually refactor this so that PropertyType is a class not an enum, so that it can have those set. That's a big change though, and we can talk about it tomorrow. If all that's needed to get this out the door now is for this actually to be an array of Propertys after all (with those redundant extra properties like descrition/name/unix_name/etc copyied over) then let's do that for now.
Started supporting 'choices' and did some rework. Forgot to do line wrapping style fixes. Will come in next patch. Also going to integrate the generated choices code somewhere to make sure it works. https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... File tools/json_schema_compiler/cc_generator.py (right): https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... tools/json_schema_compiler/cc_generator.py:213: if last != param.name: On 2012/02/06 13:14:48, kalman wrote: > Why the "last" check? Is it something to do with choices? If so, wouldn't this > be a bit dangerous to assume they all have the wrong name (see other comment). > > Yes, must be choices. That's a bummer :( > > Hmm, a few options here I suppose > - cache those values directly in the model (we can test it that way, but it > makes the model a bit bigger and that's a shame) > - tag the parameters in a way that can detect whether they're part of the same > choices group (sounds arcane and complicated; too much logic in the C > generators) > - do choices differently; push that "expansion" logic out of the model and into > the c generators. params just becomes literally the params (with a CHOICES > option) and h_generator/cc_generator/cpp_types_generator know how to expand > them. > > I reckon the last sounds the best. This method then becomes > > required = 0 > for param in function.params: > if not param.optional > required += 1 > > c = Code() > 'if %(var)s.GetSize() < %(required)d' > ' return scoped_ptr<Params>();' > 'if %(var)s.GetSize() > %(total)d' > ' return scoped_ptr<Params>();' > > return c.sub({ > 'var': var, > 'required': required, > 'total': params.length > }) Done. https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... tools/json_schema_compiler/cc_generator.py:222: check = 'if (%(var)s.GetSize() < %(req)d && %(var)s.GetSize() > %(sum)d)' On 2012/02/06 13:14:48, kalman wrote: > && should be || right? Wups, removed anyways. https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... File tools/json_schema_compiler/cpp_type_generator.py (right): https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... tools/json_schema_compiler/cpp_type_generator.py:98: if prop.item_type.type_ in (PropertyType.REF, PropertyType.ANY, PropertyType.OBJECT): On 2012/02/06 13:14:48, kalman wrote: > Cool > > Line wrapping though :) Done. https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... File tools/json_schema_compiler/model.py (right): https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... tools/json_schema_compiler/model.py:130: its model.Type On 2012/02/06 13:14:48, kalman wrote: > line wrap Done. https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... tools/json_schema_compiler/model.py:205: def _Choice(name, json): On 2012/02/06 13:14:48, kalman wrote: > I made a comment here earlier about structuring this method differently, but it > seems to have gotten lost. No matter, since I've come to realise something > after looking at that checking code. Sorry to have not have formed an opinion > about this earlier... you're going to hate me... > > but the way choices are done at the moment is hard to understand and easy to > misuse. There is logic in in the model to expand choice parameters, but not > enough (as we saw earlier) and it's not applicable to all scenarios anyway. > > To write this differently, how about: > - Have another PropertyType CHOICE. > - If a Property is CHOICE, have another member of Property called like > "choices", an array of possible Properties. > - Code that uses parameters needs to know how to expand out choices. A lot of > this, I think, should be able to be abstracted nicely away into the > CppTypeGenerator class; like a method along the lines of ExpandChoicesInParams > or something which returns an array of Properties, like function.params, but > expanded in the same way you're doing here, basically. > > (and then the h/cc generator classes in ToValue which used function.params and > assumed the Choices were taken care of would use > cpp_type_generator.ExpandChoicesInParams(function.params) instead) > > Yeah, there will be a few non-trivial changes to do something like this, but > nothing overly structural, and I think overall it will result in easier to > follow code. > > And... you'll need tests for it. Done. https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co... tools/json_schema_compiler/model.py:208: raise NotImplementedError('%s\nChoices is not fully implemented' % json) On 2012/02/06 13:14:48, kalman wrote: > Choices is implemented for return values right? Or one of them? > > I just meant to raise a NotImplementedError where you said it didn't work, not > for _all_ attempts to use choices :) > > That's a bit harsh on yourself... It's done now, but I thought it made sense to disable choices everywhere since having NotImplemented anywhere would break as soon as you had choices anyway.
Looking forward to that Choices stuff as discussed :) http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su... File generated_files_will_not_submit/tabs.h (right): http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su... generated_files_will_not_submit/tabs.h:48: // The ID of the tab that opened this tab, if any. This will only be present if looks like the comment wrapping thing got a bit broken http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su... generated_files_will_not_submit/tabs.h:139: where did this extra blank line come from? http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:109: .Eblock('}') indent 2 spaces. I can see why you might have done it this way (so that it lines up with the C++ indentation) but it looks weird. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:113: def _GenerateTypePopulateProperty(self, prop, src, dst): As discussed, most of this code will be replaced with the choices implementation in GeneratePopulateChoices; you'll just need to factor it a bit differently to deal with a Value*. Then, - this method will get the value from src[prop.name] - GeneratePopulateChoices will get it from src[index] or something like that. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:174: 'type': self._cpp_type_generator.GetType(prop) too much indentation, and spread out each member on its own line. hard to read this way. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:176: return c Can delay substituting src until the end? c.Substitute({ 'src': src }) then remove it above. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:280: return c y u no write it like I wrote? I can see why you did the num_optional thing, but those checks aren't necessary (the "if num_{optional,required}:") because they cause a weakening of the checks. Looking at permissions.cc for example, it used to be if (args.GetSize() != 1) return NULL; but now it's if (args.GetSize() < 1) return NULL; which isn't as strong. Instead, we want like (generated C++) if (args.GetSize() < 1) return NULL; if (args.GetSize() > 1) return NULL; or just if (args.GetSize() < 1 || args.GetSize() > 1) return NULL; which is the same as != 1. In fact, you could have like (Python) if num_required == params.length: c.Append('if (%(var)s.GetSize() != %(total)d') else: c.Append('if (%(var)s.GetSize() < %(required)d' ' || %(var)s.GetSize() > %(total)d') c.Append(' return scoped_ptr<Params>();') http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:345: def _GeneratePopulateChoices(self, param, src, index, return_line): confused by the use of the word "populate" here but it's actually used as part of the Params::Create() method, not Populate? http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:351: """ as discussed, refactoring to do here. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:358: value_var = param.unix_name + '_value' Refactoring being making this method accept the name of value_var rather than a ListValue and its index. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:361: c.Append(return_line) This means that optional parameters can only appear as the last parameter in function calls. You should check that the JSON behaves that way, and if it does: assert this in the model, and add a comment here. If it doesn't, then you'll need to deal with it by not returning from the function, but putting everything in an "if" block (you can figure it out I'm sure). Actually, if it's not too hard, you can save yourself and other people the cognitive effort of reading the comment(s) and reason for the assertion(s) and just implement it in the more general way to start off with. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:363: c.Substitute({'src': src, 'index': index, 'value': value_var}) IMO try to put substitutions at the end of these code blocks. When you substitute early like this, I wonder if it's because the values change later on... and here they don't. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:374: (c.Append(' %(type)s = %(enum)s;') why the extra spaces before %(type)s? http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:393: 'enum': self._cpp_type_generator.GetChoiceEnum(param, choice.type_)}) I don't know whether enum refers to the type or the value by reading the code. I think it's value, so call enum_value. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:33: def ExpandedChoicesInParams(self, params): nice though try to make method names verbs, like "GetExpandedChoicesInParams". (I didn't suggest "ExpandChoicesInParams" because that might imply that the params passed through is mutated) http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:91: assert prop.choices[type_] won't prop.choices[type_] raise a TypeError if type_ isn't there, so the assert is unnecessary? Could be missing something... http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:93: 'NONE' if type_ is None else repr(type_)) Make this two separate methods rather than overloading the method with being able to support both NONE and actual values. Like def GetChoiceEnumValue(self, prop, type_): return '%s_%s' % (prop.unix_name.upper(), type_.name) def GetChoiceEnumNoneValue(self, prop): return '%s_NONE' % (prop.unix_name_upper()) And yeah -- why the repr()? type_.name would suffice right? http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:95: def GetChoicesEnumName(self, prop): This function name is confusing me. It generates the type right? like enum GetChoicesEnumName { GetChoiceEnum(), GetChoiceEnum(), } GetChoicesEnumType would be better, I think. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:96: for prop in [x for x in props if x.type_ == PropertyType.CHOICES]: I think this needs a comment, like # Generate the enums needed for any fields with "choices". http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:105: c.Append(enum) I don't understand why you've done it this way rather than generating the code as you go, like: === enum_type = self._cpp_type_generator.GetChoicesEnumType(prop) c.Sblock('enum %s {' % enum_type) c.Append('%s,' % ...GetChoiceEnumNoneType(prop)) for choice in prop.choices.values(): s.Append('%s,' % ...GetChoiceEnumType(prop, choice.type_) c.Eblock('};') (c.Append() .Append('%s %s_type;' % (enum_type, prop.unix_name)) ) === note that c++ is ok with trailing commas on enums. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:189: unix_name = property(GetUnixName, SetUnixName) Getter/Setter can be a bit simpler to take advantage of their complementary access restrictions (you don't need the comments, I'm just adding them for illustrative purposes). === def GetUnicName(self): # There has to be a valid _unix_name since we set one in the # constructor, and SetUnixName ensures that it's only ever # changed to something valid. self._unix_name_used = True return self._unix_name def SetUnixName(self, unix_name): if unix_name is None raise AssertionErrorOrSomething(...) if self._unix_name_used raise AttributeError(...) # This is up to you, but it seems like you only want to be # able to set it once, too. self._unix_name_used = true self._unix_name = unix_name
Refactored choices and other small fixes. http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su... File generated_files_will_not_submit/tabs.h (right): http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su... generated_files_will_not_submit/tabs.h:48: // The ID of the tab that opened this tab, if any. This will only be present if On 2012/02/08 05:02:08, kalman wrote: > looks like the comment wrapping thing got a bit broken It wraps to 80 characters but then got indented 2. I could fix this.. but not now. http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su... generated_files_will_not_submit/tabs.h:139: On 2012/02/08 05:02:08, kalman wrote: > where did this extra blank line come from? Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:109: .Eblock('}') On 2012/02/08 05:02:08, kalman wrote: > indent 2 spaces. I can see why you might have done it this way (so that it lines > up with the C++ indentation) but it looks weird. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:113: def _GenerateTypePopulateProperty(self, prop, src, dst): On 2012/02/08 05:02:08, kalman wrote: > As discussed, most of this code will be replaced with the choices implementation > in GeneratePopulateChoices; you'll just need to factor it a bit differently to > deal with a Value*. Then, > - this method will get the value from src[prop.name] > - GeneratePopulateChoices will get it from src[index] > or something like that. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:174: 'type': self._cpp_type_generator.GetType(prop) On 2012/02/08 05:02:08, kalman wrote: > too much indentation, and spread out each member on its own line. hard to read > this way. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:176: return c On 2012/02/08 05:02:08, kalman wrote: > Can delay substituting src until the end? > > c.Substitute({ > 'src': src > }) > > then remove it above. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:280: return c On 2012/02/08 05:02:08, kalman wrote: > y u no write it like I wrote? > > I can see why you did the num_optional thing, but those checks aren't necessary > (the "if num_{optional,required}:") because they cause a weakening of the > checks. Looking at permissions.cc for example, it used to be > > if (args.GetSize() != 1) > return NULL; > > but now it's > > if (args.GetSize() < 1) > return NULL; > > which isn't as strong. Instead, we want like (generated C++) > > if (args.GetSize() < 1) > return NULL; > if (args.GetSize() > 1) > return NULL; > > or just > > if (args.GetSize() < 1 || args.GetSize() > 1) > return NULL; > > which is the same as != 1. In fact, you could have like (Python) > > if num_required == params.length: > c.Append('if (%(var)s.GetSize() != %(total)d') > else: > c.Append('if (%(var)s.GetSize() < %(required)d' > ' || %(var)s.GetSize() > %(total)d') > c.Append(' return scoped_ptr<Params>();') Wups, my bad. Note that you can't do args.GetSize() < 0 because C++ herps a derp about it always being true. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:345: def _GeneratePopulateChoices(self, param, src, index, return_line): On 2012/02/08 05:02:08, kalman wrote: > confused by the use of the word "populate" here but it's actually used as part > of the Params::Create() method, not Populate? Now it populates. Woo. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:351: """ On 2012/02/08 05:02:08, kalman wrote: > as discussed, refactoring to do here. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:358: value_var = param.unix_name + '_value' On 2012/02/08 05:02:08, kalman wrote: > Refactoring being making this method accept the name of value_var rather than a > ListValue and its index. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:361: c.Append(return_line) On 2012/02/08 05:02:08, kalman wrote: > This means that optional parameters can only appear as the last parameter in > function calls. You should check that the JSON behaves that way, and if it does: > assert this in the model, and add a comment here. > > If it doesn't, then you'll need to deal with it by not returning from the > function, but putting everything in an "if" block (you can figure it out I'm > sure). > > Actually, if it's not too hard, you can save yourself and other people the > cognitive effort of reading the comment(s) and reason for the assertion(s) and > just implement it in the more general way to start off with. Couldn't figure a case where requires args happen after optional args. Can't assert because there are 'optional' first args but they need to be specified as null/undefined. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:363: c.Substitute({'src': src, 'index': index, 'value': value_var}) On 2012/02/08 05:02:08, kalman wrote: > IMO try to put substitutions at the end of these code blocks. When you > substitute early like this, I wonder if it's because the values change later > on... and here they don't. Done. I'm wary of someone else changing a value in the middle of a huge code block and breaking things for the substitute at the bottom. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:374: (c.Append(' %(type)s = %(enum)s;') On 2012/02/08 05:02:08, kalman wrote: > why the extra spaces before %(type)s? Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:393: 'enum': self._cpp_type_generator.GetChoiceEnum(param, choice.type_)}) On 2012/02/08 05:02:08, kalman wrote: > I don't know whether enum refers to the type or the value by reading the code. I > think it's value, so call enum_value. Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:33: def ExpandedChoicesInParams(self, params): On 2012/02/08 05:02:08, kalman wrote: > nice > > though try to make method names verbs, like "GetExpandedChoicesInParams". > > (I didn't suggest "ExpandChoicesInParams" because that might imply that the > params passed through is mutated) Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:91: assert prop.choices[type_] On 2012/02/08 05:02:08, kalman wrote: > won't prop.choices[type_] raise a TypeError if type_ isn't there, so the assert > is unnecessary? > > Could be missing something... We don't actually use prop.choice[type_] otherwise. (unless I just use prop.choice[type_] as its own statement but that's kind of confusing too?) http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:93: 'NONE' if type_ is None else repr(type_)) On 2012/02/08 05:02:08, kalman wrote: > Make this two separate methods rather than overloading the method with being > able to support both NONE and actual values. > > Like > > def GetChoiceEnumValue(self, prop, type_): > return '%s_%s' % (prop.unix_name.upper(), type_.name) > > def GetChoiceEnumNoneValue(self, prop): > return '%s_NONE' % (prop.unix_name_upper()) > > And yeah -- why the repr()? type_.name would suffice right? Hah, wups. Yeah. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:95: def GetChoicesEnumName(self, prop): On 2012/02/08 05:02:08, kalman wrote: > This function name is confusing me. It generates the type right? like > > enum GetChoicesEnumName { > GetChoiceEnum(), > GetChoiceEnum(), > } > > GetChoicesEnumType would be better, I think. Okay. Didn't know if enums were classified as types. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:96: for prop in [x for x in props if x.type_ == PropertyType.CHOICES]: On 2012/02/08 05:02:08, kalman wrote: > I think this needs a comment, like > > # Generate the enums needed for any fields with "choices". Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:105: c.Append(enum) On 2012/02/08 05:02:08, kalman wrote: > I don't understand why you've done it this way rather than generating the code > as you go, like: > > === > > enum_type = self._cpp_type_generator.GetChoicesEnumType(prop) > c.Sblock('enum %s {' % enum_type) > > c.Append('%s,' % ...GetChoiceEnumNoneType(prop)) > for choice in prop.choices.values(): > s.Append('%s,' % ...GetChoiceEnumType(prop, choice.type_) > > c.Eblock('};') > > (c.Append() > .Append('%s %s_type;' % (enum_type, prop.unix_name)) > ) > > === > > note that c++ is ok with trailing commas on enums. Hah. Didn't know that. My default assumption is that C++ will whine. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:189: unix_name = property(GetUnixName, SetUnixName) On 2012/02/08 05:02:08, kalman wrote: > Getter/Setter can be a bit simpler to take advantage of their complementary > access restrictions (you don't need the comments, I'm just adding them for > illustrative purposes). > > === > > def GetUnicName(self): > # There has to be a valid _unix_name since we set one in the > # constructor, and SetUnixName ensures that it's only ever > # changed to something valid. > self._unix_name_used = True > return self._unix_name > > def SetUnixName(self, unix_name): > if unix_name is None > raise AssertionErrorOrSomething(...) > if self._unix_name_used > raise AttributeError(...) > # This is up to you, but it seems like you only want to be > # able to set it once, too. > self._unix_name_used = true > self._unix_name = unix_name If the property is a choice, we unset it. GetUnixName then throws the exception on attempted access. We probably don't want to set more than once but we don't really care (unless it gets used). Actually I'm going to allow this because it lets me avoid a copy in GetExpandedChoicesInParams.
Hi. I had a look at the generated stuff and added some comments. This is great stuff. I am looking forward to using it. Best, Dominic http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... File generated_files_will_not_submit/experimental.declarative.cc (right): http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.cc:86: // Rules that were registered, the optional parameters are filled with values. I think it is sufficient to have this comment in the header file. http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... File generated_files_will_not_submit/experimental.declarative.h (right): http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:10: #define CHROME_COMMON_EXTENSIONS_API_EXPERIMENTAL_DECLARATIVE_H__ Should this go to chrome/common? http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:21: using base::Value; I think the style guide prohibits "bleeding using statements" in header files http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:55: // Rule object. Passesownership to caller. nit: space after "Passes" http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:56: DictionaryValue* ToValue() const; should this return a scoped_ptr? http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:71: // Name of the event this function affects. Why is the "Name of the event this function affects." line here twice?
I have a few comments (on older drafts) that I wanted to send out before I forget and they become obsolete. =) http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:74: Use wrap_optional to wrap the type in a scoped_ptr<T> if the Property is When would you not want to use this? http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:123: c.Append('namespace %s {' % namespace.name) Any possibility of nested namespaces? http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:157: if not dependency_namespaces.get(namespace): You could use dependency_namespaces.setdefault(namespace, []) http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:33: - |source_file| the file that contained the namespace definition This seems redundant given the 2 below it. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:165: # A choice needs to have its unix_name set elsewhere Can you discuss where SetUnixName might be called from? As it is, I can't find it. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:187: 'Cannot set the unix_name on %s; it is already used elsewhere as %s') missing substitutions here? http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:213: """Returns the unix_style name for a given string. Comment here that the name is expected to be lowerCamelCase.
Pulled from master so now I'm working against Dominic's actual code. http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:74: Use wrap_optional to wrap the type in a scoped_ptr<T> if the Property is On 2012/02/08 19:04:12, Yoyo Zhou wrote: > When would you not want to use this? Anywhere that isn't a declaration. e.g scoped_ptr<X> var(new X()) would be '%s var(new %s())' % (GetType(prop, wrap_optional=True), GetType(prop)) http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:123: c.Append('namespace %s {' % namespace.name) On 2012/02/08 19:04:12, Yoyo Zhou wrote: > Any possibility of nested namespaces? I don't think so. These should only be types (e.g Window, Tab) which should all be in the top level of a generated namespace. http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c... tools/json_schema_compiler/cpp_type_generator.py:157: if not dependency_namespaces.get(namespace): On 2012/02/08 19:04:12, Yoyo Zhou wrote: > You could use dependency_namespaces.setdefault(namespace, []) Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:33: - |source_file| the file that contained the namespace definition On 2012/02/08 19:04:12, Yoyo Zhou wrote: > This seems redundant given the 2 below it. It's really just for convenience, otherwise, I'd need to import os.path and join the split version back together. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:165: # A choice needs to have its unix_name set elsewhere On 2012/02/08 19:04:12, Yoyo Zhou wrote: > Can you discuss where SetUnixName might be called from? As it is, I can't find > it. unix_name is set as a property of this class (line 189 in this patch). When the property accessed, (i.e prop.unix_name) the setter or getter is called depending on context. tl;dr python magic http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:187: 'Cannot set the unix_name on %s; it is already used elsewhere as %s') On 2012/02/08 19:04:12, Yoyo Zhou wrote: > missing substitutions here? Done. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:213: """Returns the unix_style name for a given string. On 2012/02/08 19:04:12, Yoyo Zhou wrote: > Comment here that the name is expected to be lowerCamelCase. Done. http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... File generated_files_will_not_submit/experimental.declarative.cc (right): http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.cc:86: // Rules that were registered, the optional parameters are filled with values. On 2012/02/08 09:15:20, battre wrote: > I think it is sufficient to have this comment in the header file. Done. http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... File generated_files_will_not_submit/experimental.declarative.h (right): http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:10: #define CHROME_COMMON_EXTENSIONS_API_EXPERIMENTAL_DECLARATIVE_H__ On 2012/02/08 09:15:20, battre wrote: > Should this go to chrome/common? Sorry, I'm not sure what you mean? The file is generated to a temporary directory during the build. chrome/common is simply the include path (generated from the relative path of root to the api json). http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:21: using base::Value; On 2012/02/08 09:15:20, battre wrote: > I think the style guide prohibits "bleeding using statements" in header files > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... Moved to the .cc file. http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:55: // Rule object. Passesownership to caller. On 2012/02/08 09:15:20, battre wrote: > nit: space after "Passes" Done. http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:56: DictionaryValue* ToValue() const; On 2012/02/08 09:15:20, battre wrote: > should this return a scoped_ptr? Done. http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:71: // Name of the event this function affects. On 2012/02/08 09:15:20, battre wrote: > Why is the "Name of the event this function affects." line here twice? Done.
I'm losing track of this patch a bit due to the number of changes that have happened... I think basically it LGTM after you fix up those comments. As you write those compiler-level tests and possibly find bugs, and iterate on the compiler, you might (probably will) find things to fix up. But as it stands -- this looks good and generates correct code at the moment, so cool. Make sure it passes try-jobs though. And wait for Yoyo's LGTM. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:176: return c On 2012/02/08 07:01:18, calamity wrote: > On 2012/02/08 05:02:08, kalman wrote: > > Can delay substituting src until the end? > > > > c.Substitute({ > > 'src': src > > }) > > > > then remove it above. > > Done. Not done? Also you have the same thing above now, substituting "src" in. http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:280: return c On 2012/02/08 07:01:18, calamity wrote: > On 2012/02/08 05:02:08, kalman wrote: > > y u no write it like I wrote? > > > > I can see why you did the num_optional thing, but those checks aren't > necessary > > (the "if num_{optional,required}:") because they cause a weakening of the > > checks. Looking at permissions.cc for example, it used to be > > > > if (args.GetSize() != 1) > > return NULL; > > > > but now it's > > > > if (args.GetSize() < 1) > > return NULL; > > > > which isn't as strong. Instead, we want like (generated C++) > > > > if (args.GetSize() < 1) > > return NULL; > > if (args.GetSize() > 1) > > return NULL; > > > > or just > > > > if (args.GetSize() < 1 || args.GetSize() > 1) > > return NULL; > > > > which is the same as != 1. In fact, you could have like (Python) > > > > if num_required == params.length: > > c.Append('if (%(var)s.GetSize() != %(total)d') > > else: > > c.Append('if (%(var)s.GetSize() < %(required)d' > > ' || %(var)s.GetSize() > %(total)d') > > c.Append(' return scoped_ptr<Params>();') > > Wups, my bad. Note that you can't do args.GetSize() < 0 because C++ herps a derp > about it always being true. Hah. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:123: self._util_cc_helper.GetArray(prop, src, prop.name, dst_member)) Add a comment around here saying that the C++ helper which gets the array handles required vs optional (took me a moment to remember this). http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:128: c.Append('Value* %(value)s = NULL;') It might be a good general rule, if you have local variables that end up being substituted for strings, to keep the names the same. That would make things a bit clearer. So c.Append('Value* %(value_var)s = NULL') etc. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:129: c.Append('if (!(%(src)s->Get("%(key)s", &%(value)s)))') I think you have some redundant parens around the "Get" here also, should be GetWithoutPathExpansion http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:286: return_line = ' return scoped_ptr<Params>();' eh, IMO I'd do the same kind of thing as I suggested below, and assign to a failure_value argument; a bit easier to read the Python below as C++ that way. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:291: .Append('if (!args.GetList(%(index)d, &%(var)s))') Like I commented above, call this string substitution character %(index) -> %(i), %(var) -> %(param_var) http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:305: 'return scoped_ptr<Params>();')) should be passing in return_line (aka failure_value) in here instead of scoped_ptr<Param>()? http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:333: .Eblock('}') like comment from last round, indent this in the normal python style. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:338: def _GeneratePopulateChoices(self, prop, value, dst, return_line): A few things: - Call "value" "value_var" instead, since that's what you're always calling the variable that's being passed in. - Maybe change return_line to return_value or even failure_value, and use like .Append(' return %s;' % failure_value) http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:387: 'type': type_var, Same comment as earlier; call the string substitution %(type_var) http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:83: """Gets the enum of the choices in the given model.Property indicating no "enum value" http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:89: """Gets the enum of the choices in the given model.Property of the given "enum value" http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:98: """Gets the name of the enum for the given model.Property. "Gets the type" http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:99: c.Sblock('enum %s {' % enum_name) enum_name -> enum_type (any reason you're not using Substitute in this method? just curious) http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/util.h (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:149: hm?
http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... File generated_files_will_not_submit/experimental.declarative.h (right): http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su... generated_files_will_not_submit/experimental.declarative.h:10: #define CHROME_COMMON_EXTENSIONS_API_EXPERIMENTAL_DECLARATIVE_H__ On 2012/02/09 00:56:52, calamity wrote: > On 2012/02/08 09:15:20, battre wrote: > > Should this go to chrome/common? > > Sorry, I'm not sure what you mean? The file is generated to a temporary > directory during the build. chrome/common is simply the include path (generated > from the relative path of root to the api json). I was wondering whether chrome/browser/extensions/ would be more appropriate than chrome/common/extensions/ because the API implementations are in chrome/browser/extensions.
Looks pretty good for the most part. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:281: # optional argument at position 4 cannot exist without an argument at Aaron: is mtytel's work on optional arguments to functions going to change this at all? http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a... File chrome/browser/extensions/api/permissions/permissions_api.cc (right): http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a... chrome/browser/extensions/api/permissions/permissions_api.cc:50: LOG(WARNING) << "IN HERE\n\n\n"; Are these no longer needed? http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su... File generated_files_will_not_submit/windows.cc (right): http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su... generated_files_will_not_submit/windows.cc:114: return params.Pass(); So invalid optional arguments are treated as though they are missing. But it looks like the existing behavior is to treat these as errors. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:292: .Append(return_line) I think this return_line would be different if you wanted to abort on invalid optional parameters. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:345: BEFORE you call this method as return_line will be used to indicate a Maybe s/you call/the code generated by/ http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:359: PropertyType.STRING: 'Value::TYPE_STRING', These "Value::..." look like they could be attributes of PropertyType, or perhaps the PropertyType name could be used here. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:414: for param in self._cpp_type_generator.GetExpandedChoicesInParams(params): This logic looks somewhat duplicated between here and the h_generator. Consider a utility function that you could use in both places. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:94: for prop in [x for x in props if x.type_ == PropertyType.CHOICES]: Why not just for prop in props: if prop.type__ == ...: http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:164: # A choice needs to have its unix_name set elsewhere Ok. What I meant was "elsewhere" is vague. I found GetExpandedChoicesInParams -- it would help understanding to have it in the comment here. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:192: unix_name = property(GetUnixName, SetUnixName) Ah, this line (the python magic) was hiding here. It should probably have a newline above it. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/util.h (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:40: // Creates a new vector containing Llist| at |out|. Returns typo: L -> | Also, this doesn't appear to create a new vector. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:129: // Set |out| to the the contents of |from|. Requires GetItemFromList to be This one doesn't appear to erase |out|'s existing contents. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:132: void SetArrayToList( This name is a little awkward. I'd think it means array = list; when it's actually vice versa. What about AddArrayItemsToList or some such? Maybe ...From...? Same for the names below. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/util_cc_helper.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util_cc_helper.py:60: """Sets dst.name to the array at src Generates code to...
http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:123: self._util_cc_helper.GetArray(prop, src, prop.name, dst_member)) On 2012/02/09 02:49:36, kalman wrote: > Add a comment around here saying that the C++ helper which gets the array > handles required vs optional (took me a moment to remember this). Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:128: c.Append('Value* %(value)s = NULL;') On 2012/02/09 02:49:36, kalman wrote: > It might be a good general rule, if you have local variables that end up being > substituted for strings, to keep the names the same. That would make things a > bit clearer. > > So c.Append('Value* %(value_var)s = NULL') etc. Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:129: c.Append('if (!(%(src)s->Get("%(key)s", &%(value)s)))') On 2012/02/09 02:49:36, kalman wrote: > I think you have some redundant parens around the "Get" here > > also, should be GetWithoutPathExpansion Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:286: return_line = ' return scoped_ptr<Params>();' On 2012/02/09 02:49:36, kalman wrote: > eh, IMO I'd do the same kind of thing as I suggested below, and assign to a > failure_value argument; a bit easier to read the Python below as C++ that way. Done. I wanted to Substitute it but the line is everywhere and python will complain if you try to partially substitute a string so I couldn't find a nice place to do the substitution. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:291: .Append('if (!args.GetList(%(index)d, &%(var)s))') On 2012/02/09 02:49:36, kalman wrote: > Like I commented above, call this string substitution character %(index) -> > %(i), %(var) -> %(param_var) Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:305: 'return scoped_ptr<Params>();')) On 2012/02/09 02:49:36, kalman wrote: > should be passing in return_line (aka failure_value) in here instead of > scoped_ptr<Param>()? By this point we've done our optional check. Anything that goes wrong here is a proper failure. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:333: .Eblock('}') On 2012/02/09 02:49:36, kalman wrote: > like comment from last round, indent this in the normal python style. Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:338: def _GeneratePopulateChoices(self, prop, value, dst, return_line): On 2012/02/09 02:49:36, kalman wrote: > A few things: > > - Call "value" "value_var" instead, since that's what you're always calling the > variable that's being passed in. > > - Maybe change return_line to return_value or even failure_value, and use like > .Append(' return %s;' % failure_value) Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:387: 'type': type_var, On 2012/02/09 02:49:36, kalman wrote: > Same comment as earlier; call the string substitution %(type_var) Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:83: """Gets the enum of the choices in the given model.Property indicating no On 2012/02/09 02:49:36, kalman wrote: > "enum value" Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:89: """Gets the enum of the choices in the given model.Property of the given On 2012/02/09 02:49:36, kalman wrote: > "enum value" Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/cpp_type_generator.py:98: """Gets the name of the enum for the given model.Property. On 2012/02/09 02:49:36, kalman wrote: > "Gets the type" Done. http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:99: c.Sblock('enum %s {' % enum_name) On 2012/02/09 02:49:36, kalman wrote: > enum_name -> enum_type > > (any reason you're not using Substitute in this method? just curious) Done. Just got lazy I guess. http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a... File chrome/browser/extensions/api/permissions/permissions_api.cc (right): http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a... chrome/browser/extensions/api/permissions/permissions_api.cc:50: LOG(WARNING) << "IN HERE\n\n\n"; On 2012/02/10 01:49:33, Yoyo Zhou wrote: > Are these no longer needed? Done. http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su... File generated_files_will_not_submit/windows.cc (right): http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su... generated_files_will_not_submit/windows.cc:114: return params.Pass(); On 2012/02/10 01:49:33, Yoyo Zhou wrote: > So invalid optional arguments are treated as though they are missing. But it > looks like the existing behavior is to treat these as errors. Good catch. This is a big-ish change. I think it should go in the next patch (which will have badass tests which can actually check for this sort of behavior). http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:292: .Append(return_line) On 2012/02/10 01:49:33, Yoyo Zhou wrote: > I think this return_line would be different if you wanted to abort on invalid > optional parameters. Agreed. I want to do a pretty major refactor regarding this for my next patch so I'll just leave it til then? http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:345: BEFORE you call this method as return_line will be used to indicate a On 2012/02/10 01:49:33, Yoyo Zhou wrote: > Maybe s/you call/the code generated by/ Done. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:359: PropertyType.STRING: 'Value::TYPE_STRING', On 2012/02/10 01:49:33, Yoyo Zhou wrote: > These "Value::..." look like they could be attributes of PropertyType, or > perhaps the PropertyType name could be used here. I feel that this would couple the python code too closely to the C++ code? Using the PropertyType.name for names that we generate is one thing, using it to refer to code that isn't ours seems dodgy. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:414: for param in self._cpp_type_generator.GetExpandedChoicesInParams(params): On 2012/02/10 01:49:33, Yoyo Zhou wrote: > This logic looks somewhat duplicated between here and the h_generator. Consider > a utility function that you could use in both places. Done. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:94: for prop in [x for x in props if x.type_ == PropertyType.CHOICES]: On 2012/02/10 01:49:33, Yoyo Zhou wrote: > Why not just > for prop in props: > if prop.type__ == ...: Hah, that's what is was before. I thought it'd be more python-y to use a list comprehension. Reverted. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/model.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:164: # A choice needs to have its unix_name set elsewhere On 2012/02/10 01:49:33, Yoyo Zhou wrote: > Ok. What I meant was "elsewhere" is vague. I found GetExpandedChoicesInParams > -- it would help understanding to have it in the comment here. Done. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/model.py:192: unix_name = property(GetUnixName, SetUnixName) On 2012/02/10 01:49:33, Yoyo Zhou wrote: > Ah, this line (the python magic) was hiding here. It should probably have a > newline above it. Done. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/util.h (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:40: // Creates a new vector containing Llist| at |out|. Returns On 2012/02/10 01:49:33, Yoyo Zhou wrote: > typo: L -> | > Also, this doesn't appear to create a new vector. Done. Also erases |out|. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:129: // Set |out| to the the contents of |from|. Requires GetItemFromList to be On 2012/02/10 01:49:33, Yoyo Zhou wrote: > This one doesn't appear to erase |out|'s existing contents. Done. Erases now. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:132: void SetArrayToList( On 2012/02/10 01:49:33, Yoyo Zhou wrote: > This name is a little awkward. I'd think it means > array = list; > when it's actually vice versa. > What about AddArrayItemsToList or some such? Maybe ...From...? > > Same for the names below. You're right. Since I added the erasure of |out|, I didn't want to use Add in the name. I chose Populate instead? Let me know if you think of something better. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/util_cc_helper.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util_cc_helper.py:60: """Sets dst.name to the array at src On 2012/02/10 01:49:33, Yoyo Zhou wrote: > Generates code to... Done.
LGTM http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:292: .Append(return_line) On 2012/02/10 03:52:50, calamity wrote: > On 2012/02/10 01:49:33, Yoyo Zhou wrote: > > I think this return_line would be different if you wanted to abort on invalid > > optional parameters. > > Agreed. I want to do a pretty major refactor regarding this for my next patch so > I'll just leave it til then? Ok, just add a TODO for it. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/cc_generator.py:359: PropertyType.STRING: 'Value::TYPE_STRING', On 2012/02/10 03:52:50, calamity wrote: > On 2012/02/10 01:49:33, Yoyo Zhou wrote: > > These "Value::..." look like they could be attributes of PropertyType, or > > perhaps the PropertyType name could be used here. > > I feel that this would couple the python code too closely to the C++ code? Using > the PropertyType.name for names that we generate is one thing, using it to refer > to code that isn't ours seems dodgy. Ok, makes sense. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/h_generator.py (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/h_generator.py:94: for prop in [x for x in props if x.type_ == PropertyType.CHOICES]: On 2012/02/10 03:52:50, calamity wrote: > On 2012/02/10 01:49:33, Yoyo Zhou wrote: > > Why not just > > for prop in props: > > if prop.type__ == ...: > > Hah, that's what is was before. I thought it'd be more python-y to use a list > comprehension. Reverted. It is more python-y if it becomes less code, but in this case you're just iterating over the values, not making a list. http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... File tools/json_schema_compiler/util.h (right): http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/... tools/json_schema_compiler/util.h:132: void SetArrayToList( On 2012/02/10 03:52:50, calamity wrote: > On 2012/02/10 01:49:33, Yoyo Zhou wrote: > > This name is a little awkward. I'd think it means > > array = list; > > when it's actually vice versa. > > What about AddArrayItemsToList or some such? Maybe ...From...? > > > > Same for the names below. > > You're right. Since I added the erasure of |out|, I didn't want to use Add in > the name. I chose Populate instead? Let me know if you think of something > better. All the populating looks pretty consistent.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/9309044/34001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/9309044/42001
Change committed as 122082 |
