Add optional schema compiler error messages

tools/json_schema_compiler/h_generator.py

Index: tools/json_schema_compiler/h_generator.py
diff --git a/tools/json_schema_compiler/h_generator.py b/tools/json_schema_compiler/h_generator.py
index 7cf6836fbf00ae8d92d79d166c46c056ea7c0665..3eca5a7e4cf52cd22e3ded29fbca8b4e2cc06a65 100644
--- a/tools/json_schema_compiler/h_generator.py
+++ b/tools/json_schema_compiler/h_generator.py
@@ -26,6 +26,9 @@ class _Generator(object):
     self._cpp_namespace = cpp_namespace
     self._target_namespace = (
         self._type_helper.GetCppNamespaceName(self._namespace))
+    self._error_parameter = ''
+    if self._namespace.generate_error_messages:
+      self._error_parameter = ', std::string* error = NULL'

[not at google - send to devlin, 2013/06/11 18:33:43]

just make it take an required parameter error, no optional parameter. there
won't be much code to update.

[Aaron Jacobs, 2013/06/14 02:00:37]

tools/json_schema_compiler/util is dependent on the function signature, and
would need to change depending on the |compiler_options|

[not at google - send to devlin, 2013/06/14 02:14:01]

that's ok. Just pass through NULL or something in the case when we're not
generating it.

[Aaron Jacobs, 2013/06/14 21:11:30]

While that will work for this situation, there is a larger issue - if I make the
parameter required, all calls to |FromValue| or |Populate| anywhere within the
chromium codebase will need to be changed to pass in a NULL argument.

[not at google - send to devlin, 2013/06/14 21:14:15]

Why? Still generate the correct Populate, just in the no-error mode make it pass
NULL.

[Aaron Jacobs, 2013/06/14 21:21:24]

If the parameter is not optional, then the function signature between extensions
with error messages and those without are different, which causes issues if they
reference eachother (e.g. chrome/common/extensions/api/extension.cc:48, which
references tabs). In order to keep them compatible, either all calls need to
have the parameter, all need to not have the parameter, or the parameter needs
to be optional.

[not at google - send to devlin, 2013/06/14 21:48:39]

I see. jyasskin@ is there an idiom for this? It seems wasteful to always
generate two constructors. Is an optional parameter justified in this case?

[Jeffrey Yasskin, 2013/06/14 21:58:47]

I see duplicate functions but not duplicate constructors. Lawrence Crowl argued
for the current rule, that overloaded functions should be preferred over default
arguments, and I don't think the reasoning is actually compelling, but for
everything but constructors it's easy and efficient to just generate the inline
forwarding function. For constructors, I'd use a default argument.

 
   def Generate(self):
     """Generates a Code object with the .h for a single namespace.
@@ -228,14 +231,15 @@ class _Generator(object):
           .Comment('Populates a %s object from a base::Value. Returns'
                    ' whether |out| was successfully populated.' % classname)
           .Append('static bool Populate(const base::Value& value, '
-                  '%(classname)s* out);')
+                  '%s* out%s);' % (classname, self._error_parameter))

[not at google - send to devlin, 2013/06/11 18:33:43]

perhaps:

  params = ['const base::Value& value', '%s out' % classname]
  if self._GenerateErrorMessages():
    params.append('std::string* error')
  ...
  .Append('static bool Populate(%s);' % ', '.join(params))

which involves adding a method _GenerateErrorMessages() which would look up the
compiler options.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

         )
         if is_toplevel:
           (c.Append()
             .Comment('Creates a %s object from a base::Value, or NULL on '
                      'failure.' % classname)
-            .Append('static scoped_ptr<%(classname)s> '
-                        'FromValue(const base::Value& value);')
+            .Append('static scoped_ptr<%s> FromValue('
+                    'const base::Value& value%s);' %
+                        (classname, self._error_parameter))

[not at google - send to devlin, 2013/06/11 18:33:43]

likewise. maybe there is a nice way to share that logic here and above and in
cc_generator.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

           )
       if type_.origin.from_client:
         (c.Append()
@@ -279,7 +283,7 @@ class _Generator(object):
         (c.Comment('Populates a %s object from a base::Value. Returns'
                    ' whether |out| was successfully populated.' % classname)
           .Append('static bool Populate(const base::Value& value, '
-                  '%(classname)s* out);')
+                  '%s* out%s);' % (classname, self._error_parameter))

[not at google - send to devlin, 2013/06/11 18:33:43]

ditto

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

           .Append()
         )
       if type_.origin.from_client:
@@ -338,7 +342,8 @@ class _Generator(object):
 
     c = Code()
     (c.Sblock('struct Params {')
-      .Append('static scoped_ptr<Params> Create(const base::ListValue& args);')
+      .Append('static scoped_ptr<Params> Create('
+              'const base::ListValue& args%s);' % self._error_parameter)

[not at google - send to devlin, 2013/06/11 18:33:43]

ditto

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

       .Append('~Params();')
       .Append()
       .Cblock(self._GenerateTypes(p.type_ for p in function.params))