Bug 1564724 - Generate StaticPrefList.h from StaticPrefList.yaml. r=glandium

This commit introduces StaticPrefList.yaml, which encodes the same information
as StaticPrefList.h. The .yaml file was generated with a script, which is not
part of this commit because it only needs to be used once. (I will attach it to
the bug, however.)

The commit doesn't remove StaticPrefList.h, I will do that in the next commit.
(This makes things it easier to rerun the header-to-YAML script if/when
necessary.) The commit does modify the comment at the top of StaticPrefList.h;
that modified comment can also be seen at the top of StaticPrefList.yaml.

This commit also adds a script that converts the YAML to a header file. This
script becomes part of the build.

I have done my best to verify that the conversion is correct by comparing the
original .h file with the one generated from the YAML file. They are identical,
modulo removed comments and the processing of preprocessor directives.

Differential Revision: https://phabricator.services.mozilla.com/D37526

--HG--
extra : moz-landing-system : lando

modules/libpref/init/StaticPrefList.h

@@ -10,9 +10,7 @@
 // The file is separated into sections, where the sections are determined by
 // the first segment of the prefnames within (e.g. "network.predictor.enabled"
 // is within the `Prefs starting with "network."` section). Sections must be
-// kept in alphabetical order, but prefs within sections need not be. Please
-// follow the existing naming convention when considering adding a new pref and
-// whether you need a new section.
+// kept in alphabetical order, but prefs within sections need not be.
 //
 // Basics
 // ------
@@ -28,88 +26,87 @@
 // new pref, and don't invent a new first segment unless it's appropriate and
 // there are likely to be multiple prefs with that same first segment.
 //
-// Normal prefs
+// Definitions
+// -----------
+// A pref definition looks like this:
+//
+//   - name: <pref-name>                    # mandatory
+//     type: <cpp-type>                     # mandatory
+//     value: <default-value>               # mandatory
+//     mirror: <never | once | live>        # mandatory
+//     do_not_use_directly: <True | False>  # optional
+//
+// - `name` is the name of the pref, without double-quotes, as it appears
+//   in about:config. It is used in most libpref API functions (from both C++
+//   and JS code).
+//
+// - `type` is one of `bool`, `int32_t`, `uint32_t`, `float`, an atomic version
+//   of one of those, or `String`. Note that float prefs are stored internally
+//   as strings. The C++ preprocessor doesn't like template syntax in a macro
+//   argument, so use the typedefs defined in StaticPrefsBase.h; for example,
+//   use `RelaxedAtomicBool` instead of `Atomic<bool, Relaxed>`.
+//
+// - `value` is the default value. Its type should be appropriate for
+//   <cpp-type>, otherwise the generated code will fail to compile. A complex
+//   C++ numeric expressions like `60 * 60` (which the YAML parser cannot treat
+//   as an integer or float) is treated as a string and passed through without
+//   change, which is useful.
+//
+// - `mirror` indicates how the pref value is mirrored into a C++ variable.
+//
+//   * `never`: There is no global variable mirror. The pref value can only be
+//     accessed via the standard libpref API functions.
+//
+//   * `once`: The pref value is mirrored into a variable at startup; the
+//     mirror variable is left unchanged after that. (The exact point at which
+//     all `once` mirror variables are set is when the first `once` mirror
+//     variable is accessed, via its getter function.) This is mostly useful
+//     for graphics prefs where we often don't want a new pref value to apply
+//     until restart. Otherwise, this update policy is best avoided because its
+//     behaviour can cause confusion and bugs.
+//
+//   * `always`: The mirror value is always kept in sync with the pref value.
+//     This is the most common choice.
+//
+//   The getter function's name is the same as the pref's name, but with '.' or
+//   '-' chars converted to '_', to make a valid identifier. For example, the
+//   getter for `foo.bar_baz` is `foo_bar_baz()`. This is ugly but clear, and
+//   you can search for both the pref name and the getter using the regexp
+//   /foo.bar.baz/.
+//
+//   Using the getter function to read the pref's value has the two following
+//   advantages over the normal API functions.
+//
+//   * A direct variable access is faster than a hash table lookup.
+//
+//   * A variable can be accessed off the main thread. If a pref *is* accessed
+//     off the main thread, it should have an atomic type. Assertions enforce
+//     this.
+//
+//   Note that Rust code must access the global variable directly, rather than
+//   via the getter.
+//
+// - `do_not_use_directly` dictates if `_do_not_use_directly` should be
+//   appended to the name of the getter function. This is simply a naming
+//   convention indicating that there is some other wrapper getter function
+//   that should be used in preference to the normal static pref getter.
+//   Defaults to `false` if not present. Cannot be used with a `never` mirror
+//   value, because there is no getter function in that case.
+//
+// Preprocessor
 // ------------
-// Definitions of normal prefs in this file have the following form.
+// Note finally that this file is preprocessed by preprocessor.py, not the C++
+// preprocessor. As a result, the following things may be surprising.
 //
-//   PREF(<pref-name-string>, <cpp-type>, <default-value>)
+// - YAML comments start with a '#', so putting a comment on the same line as a
+//   preprocessor directive is dubious. E.g. avoid lines like `#define X 3 #
+//   three` because the ` # three` will be part of `X`.
 //
-// - <pref-name-string> is the name of the pref, as it appears in about:config.
-//   It is used in most libpref API functions (from both C++ and JS code).
+// - '@' use is required for substitutions to occur. E.g. with `#define FOO 1`,
+//   `FOO` won't be replaced with `1` unless it has '@' chars around it.
 //
-// - <cpp-type> is one of bool, int32_t, float, or String (which is just a
-//   typedef for `const char*` in StaticPrefs.h). Note that float prefs are
-//   stored internally as strings.
-//
-// - <default-value> is the default value. Its type should match <cpp-type>.
-//
-// VarCache prefs
-// --------------
-// A VarCache pref is a special type of pref. It can be accessed via the normal
-// pref hash table lookup functions, but it also has:
-//
-// - an associated global variable (the VarCache) that mirrors the pref value
-//   in the prefs hash table (unless the update policy is `Once`, see below);
-//   and
-//
-// - a getter function that reads that global variable.
-//
-// Using the getter to read the pref's value has the two following advantages
-// over the normal API functions.
-//
-// - A direct global variable access is faster than a hash table lookup.
-//
-// - A global variable can be accessed off the main thread. If a pref *is*
-//   accessed off the main thread, it should use an atomic type. (But note that
-//   many VarCaches that should be atomic are not, in particular because
-//   Atomic<float> is not available, alas.)
-//
-// Definitions of VarCache prefs in this file has the following form.
-//
-//   VARCACHE_PREF(
-//     <update-policy>,
-//     <pref-name-string>,
-//      <pref-name-id>,  // indented one space to align with <pref-name-string>
-//     <cpp-type>, <default-value>
-//   )
-//
-// - <update-policy> is one of the following:
-//
-//   * Live: Evaluate the pref and set callback so it stays current/live. This
-//     is the normal policy.
-//
-//   * Once: Set the value once at startup, and then leave it unchanged after
-//     that. (The exact point at which all Once pref values is set is when the
-//     first Once getter is called.) This is useful for graphics prefs where we
-//     often don't want a new pref value to apply until restart. Otherwise, this
-//     update policy is best avoided because its behaviour can cause confusion
-//     and bugs.
-//
-// - <pref-name-string> is the same as for normal prefs.
-//
-// - <pref-name-id> is the name of the static getter function generated within
-//   the StaticPrefs class. For consistency, the identifier for every pref
-//   should be created by starting with <pref-name-string> and converting any
-//   '.' or '-' chars to '_'. For example, "foo.bar_baz" becomes
-//   `foo_bar_baz`. This is arguably ugly, but clear, and you can search for
-//   both using the regexp /foo.bar.baz/. Some getter functions have
-//   `_do_not_use_directly` appended to indicate that there is some other
-//   wrapper getter that should be used in preference to the normal static pref
-//   getter.
-//
-// - <cpp-type> is one of bool, int32_t, uint32_t, float, or an Atomic version
-//   of one of those. The C++ preprocessor doesn't like template syntax in a
-//   macro argument, so use the typedefs defines in StaticPrefs.h; for example,
-//   use `ReleaseAcquireAtomicBool` instead of `Atomic<bool, ReleaseAcquire>`.
-//   A pref with a `Once` policy should be non-atomic as it is only ever
-//   written to once during the parent process startup. A pref with a Live
-//   policy must be made Atomic if ever accessed outside the main thread;
-//   assertions are in place to ensure this.
-//
-// - <default-value> is the same as for normal prefs.
-//
-// Note that Rust code must access the global variable directly, rather than via
-// the getter.
+// - Spaces aren't permitted between the leading '#' and the name of a
+//   directive, e.g. `#ifdef XYZ` works but `# ifdef XYZ` does not.
 
 // clang-format off
 

modules/libpref/init/generate_static_pref_list.py

@@ -0,0 +1,185 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from __future__ import print_function
+import buildconfig
+import sys
+import yaml
+from mozbuild.preprocessor import Preprocessor
+from io import BytesIO
+
+valid_keys = {
+    'name',
+    'type',
+    'value',
+    'mirror',
+    'do_not_use_directly',
+}
+
+valid_mirrors = {
+    'never',
+    'once',
+    'always'
+}
+
+valid_bool_types = {
+    'bool',
+    # These ones are defined in StaticPrefsBase.h.
+    'RelaxedAtomicBool',
+    'ReleaseAcquireAtomicBool',
+    'SequentiallyConsistentAtomicBool',
+}
+
+valid_types = valid_bool_types.union({
+    'int32_t',
+    'uint32_t',
+    'float',
+    # These ones are defined in StaticPrefsBase.h.
+    'String',
+    'RelaxedAtomicInt32',
+    'RelaxedAtomicUint32',
+    'ReleaseAcquireAtomicInt32',
+    'ReleaseAcquireAtomicUint32',
+    'SequentiallyConsistentAtomicInt32',
+    'SequentiallyConsistentAtomicUint32',
+    'AtomicFloat',
+})
+
+header_template = '''\
+// This file was autogenerated by generate_static_pref_list.py. DO NOT EDIT.
+'''
+
+mirror_templates = {
+    'never': '''\
+PREF("{name}", {typ}, {value})
+''',
+
+    'once': '''\
+VARCACHE_PREF(
+  Once,
+  "{name}",
+   {id},
+  {typ}, {value}
+)
+''',
+
+    'always': '''\
+VARCACHE_PREF(
+  Live,
+  "{name}",
+   {id},
+  {typ}, {value}
+)
+''',
+}
+
+
+def error(msg):
+    raise ValueError(msg)
+
+
+def pref_id(pref):
+    if pref['mirror'] == 'never':
+        if pref.get('do_not_use_directly'):
+            error('`do_not_use_directly` uselessly set with `mirror` value '
+                  '`never` for pref `{}`'.format(pref['name']))
+        return None
+
+    id = pref['name'].replace('.', '_').replace('-', '_')
+    if pref.get('do_not_use_directly'):
+        id += '_do_not_use_directly'
+    return id
+
+
+def generate_header(pref_list):
+    lines = [header_template]
+
+    prev_pref = None
+    for pref in pref_list:
+        # Check all given keys are known ones.
+        for key in pref:
+            if key not in valid_keys:
+                error('invalid key `{}`'.format(key))
+
+        # 'name' must be present, valid, and in the right section.
+        if 'name' not in pref:
+            error('missing `name` key')
+        name = pref['name']
+        if type(name) != str:
+            error('non-string `name` value `{}`'.format(name))
+        if '.' not in name:
+            error('`name` value `{}` lacks a \'.\''.format(name))
+        if prev_pref:
+            prev_pref_prefix = prev_pref['name'].partition('.')[0]
+            if prev_pref_prefix > name:
+                error('`{}` pref must come before `{}` pref'
+                      .format(name, prev_pref['name']))
+
+        # 'type' must be present and valid.
+        if 'type' not in pref:
+            error('missing `type` key for pref `{}`'.format(name))
+        typ = pref['type']
+        if typ not in valid_types:
+            error('invalid `type` value `{}` for pref `{}`'.format(typ, name))
+
+        # 'value' must be present and valid.
+        if 'value' not in pref:
+            error('missing `value` key for pref `{}`'.format(name))
+        value = pref['value']
+        if typ == 'String':
+            if type(value) != str:
+                error('non-string value `{}` for `String` pref `{}`; '
+                      'add double quotes'
+                      .format(value, name))
+            # Quote string literals, and escape double-quote chars.
+            value = '"{}"'.format(value.replace('"', '\\"'))
+        elif typ in valid_bool_types:
+            if value is True:               # Convert Python bools to C++ bools.
+                value = 'true'
+            elif value is False:
+                value = 'false'
+            else:
+                error('invalid boolean value `{}` for pref `{}`'
+                      .format(value, name))
+
+        # 'mirror' must be present and valid.
+        if 'mirror' not in pref:
+            error('missing `mirror` key for pref `{}`'.format(name))
+        mirror = pref['mirror']
+        if mirror not in mirror_templates:
+            error('invalid `mirror` value `{}` for pref `{}`'
+                  .format(mirror, name))
+
+        # Generate the C++ code.
+        lines.append(mirror_templates[mirror].format(
+            name=name,
+            id=pref_id(pref),
+            typ=typ,
+            value=value,
+        ))
+
+        prev_pref = pref
+
+    return '\n'.join(lines)
+
+
+def emit_header(output, pref_list_filename):
+    pp = Preprocessor()
+    pp.context.update(buildconfig.defines['ALLDEFINES'])
+
+    # A necessary hack until MOZ_DEBUG_FLAGS are part of buildconfig.defines.
+    if buildconfig.substs.get('MOZ_DEBUG'):
+        pp.context['DEBUG'] = '1'
+
+    pp.out = BytesIO()
+    pp.do_filter('substitution')
+    pp.do_include(pref_list_filename)
+
+    try:
+        pref_list = yaml.safe_load(pp.out.getvalue())
+        output.write('{}'.format(generate_header(pref_list)))
+    except (IOError, ValueError) as e:
+        print('{}: error:\n  {}\n'
+              .format(pref_list_filename, e))
+        sys.exit(1)

modules/libpref/moz.build

@@ -25,7 +25,7 @@ XPIDL_SOURCES += [
 XPIDL_MODULE = 'pref'
 
 EXPORTS.mozilla += [
-    'init/StaticPrefList.h',
+    '!init/StaticPrefList.h',
     'init/StaticPrefList_accessibility.h',
     'init/StaticPrefListAll.h',
     'init/StaticPrefListBegin.h',
@@ -37,11 +37,25 @@ EXPORTS.mozilla += [
     'StaticPrefsBase.h',
 ]
 
+GENERATED_FILES += [
+    'init/StaticPrefList.h',
+]
+
 UNIFIED_SOURCES += [
     'Preferences.cpp',
     'SharedPrefMap.cpp',
 ]
 
+static_pref_list = GENERATED_FILES['init/StaticPrefList.h']
+static_pref_list.script = 'init/generate_static_pref_list.py:emit_header'
+static_pref_list.inputs = [
+    'init/StaticPrefList.yaml',
+]
+
+PYTHON_UNITTEST_MANIFESTS += [
+    'test/python.ini',
+]
+
 XPCOM_MANIFESTS += [
     'components.conf',
 ]

modules/libpref/test/python.ini

@@ -0,0 +1,3 @@
+[DEFAULT]
+
+[test_generate_static_pref_list.py]

modules/libpref/test/test_generate_static_pref_list.py

@@ -0,0 +1,254 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from __future__ import absolute_import, print_function
+
+import mozpack.path as mozpath
+import mozunit
+import sys
+import unittest
+import yaml
+from os import path
+from StringIO import StringIO
+
+sys.path.append(path.join(path.dirname(__file__), ".."))
+from init.generate_static_pref_list import generate_header
+
+test_data_path = mozpath.abspath(mozpath.dirname(__file__))
+test_data_path = mozpath.join(test_data_path, 'data')
+
+# A single good input with lots of different combinations.
+good_input = '''
+- name: my.bool
+  type: bool
+  value: false
+  mirror: never
+
+- name: my.int
+  type: int32_t
+  value: -123
+  mirror: once
+  do_not_use_directly: false
+
+- mirror: always
+  value: 999
+  type: uint32_t
+  name: my.uint
+
+- name: my.float            # A comment.
+  type: float               # A comment.
+  do_not_use_directly: true # A comment.
+  value: 0.0f               # A comment.
+  mirror: once              # A comment.
+
+# A comment.
+- name: my.string
+  type: String
+  value: foo"bar    # The double quote needs escaping.
+  mirror: never
+
+# A comment.
+- name: my.string2
+  type: String
+  value: "foobar"    # This string is quoted.
+  mirror: never
+
+# A comment.
+- name: my.atomic.bool
+  type: RelaxedAtomicBool
+  value: true
+  mirror: always
+
+# YAML+Python interprets `10 + 10 * 20` as a string, and so it is printed
+# unchanged.
+- name: my.atomic.int
+  type: ReleaseAcquireAtomicInt32
+  value: 10 + 10 * 20
+  mirror: always
+  do_not_use_directly: true # A comment.
+
+# YAML+Python changes `0x44` to `68` because it interprets the value as an
+# integer.
+- name: my.atomic.uint
+  type: SequentiallyConsistentAtomicUint32
+  value: 0x44
+  mirror: once
+
+# YAML+Python changes `.4455667` to `0.4455667` because it interprets the value
+# as a float.
+- name: my.atomic.float
+  type: AtomicFloat
+  value: .4455667
+  mirror: never
+'''
+
+# The corresponding output for good_input.
+good_output = '''\
+// This file was autogenerated by generate_static_pref_list.py. DO NOT EDIT.
+
+PREF("my.bool", bool, false)
+
+VARCACHE_PREF(
+  Once,
+  "my.int",
+   my_int,
+  int32_t, -123
+)
+
+VARCACHE_PREF(
+  Live,
+  "my.uint",
+   my_uint,
+  uint32_t, 999
+)
+
+VARCACHE_PREF(
+  Once,
+  "my.float",
+   my_float_do_not_use_directly,
+  float, 0.0f
+)
+
+PREF("my.string", String, "foo\\"bar")
+
+PREF("my.string2", String, "foobar")
+
+VARCACHE_PREF(
+  Live,
+  "my.atomic.bool",
+   my_atomic_bool,
+  RelaxedAtomicBool, true
+)
+
+VARCACHE_PREF(
+  Live,
+  "my.atomic.int",
+   my_atomic_int_do_not_use_directly,
+  ReleaseAcquireAtomicInt32, 10 + 10 * 20
+)
+
+VARCACHE_PREF(
+  Once,
+  "my.atomic.uint",
+   my_atomic_uint,
+  SequentiallyConsistentAtomicUint32, 68
+)
+
+PREF("my.atomic.float", AtomicFloat, 0.4455667)
+'''
+
+# A lot of bad inputs, each with an accompanying error message. Listed in order
+# of the relevant `error` calls within generate_static_pref_list.py.
+bad_inputs = [
+    ('''
+- name: do_not_use_directly.uselessly.set
+  type: int32_t
+  value: 0
+  mirror: never
+  do_not_use_directly: true
+''', '`do_not_use_directly` uselessly set with `mirror` value `never` for '
+        'pref `do_not_use_directly.uselessly.set`'),
+
+    ('''
+- invalidkey: 3
+''', 'invalid key `invalidkey`'),
+
+    ('''
+- type: int32_t
+''', 'missing `name` key'),
+
+    ('''
+- name: 99
+''', 'non-string `name` value `99`'),
+
+    ('''
+- name: name_with_no_dot
+''', '`name` value `name_with_no_dot` lacks a \'.\''),
+
+    ('''
+- name: your.pref
+  type: bool
+  value: false
+  mirror: never
+- name: my.pref
+  type: bool
+  value: false
+  mirror: never
+''', '`my.pref` pref must come before `your.pref` pref'),
+
+    ('''
+- name: missing.type.key
+  value: false
+  mirror: never
+''', 'missing `type` key for pref `missing.type.key`'),
+
+    ('''
+- name: invalid.type.value
+  type: const char*
+  value: true
+  mirror: never
+''', 'invalid `type` value `const char*` for pref `invalid.type.value`'),
+
+    ('''
+- name: missing.value.key
+  type: int32_t
+  mirror: once
+''', 'missing `value` key for pref `missing.value.key`'),
+
+    ('''
+- name: non.string
+  type: String
+  value: 3.45
+  mirror: once
+''', 'non-string value `3.45` for `String` pref `non.string`; add double quotes'),
+
+    ('''
+- name: invalid.boolean.value
+  type: bool
+  value: true || false
+  mirror: once
+''', 'invalid boolean value `true || false` for pref `invalid.boolean.value`'),
+
+    ('''
+- name: missing.mirror.key
+  type: int32_t
+  value: 3
+''', 'missing `mirror` key for pref `missing.mirror.key`'),
+
+    ('''
+- name: invalid.mirror.value
+  type: bool
+  value: true
+  mirror: sometimes
+''', 'invalid `mirror` value `sometimes` for pref `invalid.mirror.value`'),
+]
+
+
+class TestGenerateStaticPrefList(unittest.TestCase):
+    '''
+    Unit tests for generate_static_pref_list.py.
+    '''
+
+    def test_good(self):
+        'Test various pieces of good input.'
+        inp = StringIO(good_input)
+        pref_list = yaml.safe_load(inp)
+        actual = generate_header(pref_list)
+        self.assertEqual(good_output, actual)
+
+    def test_bad(self):
+        'Test various pieces of bad input.'
+
+        for (input_string, expected) in bad_inputs:
+            inp = StringIO(input_string)
+            try:
+                pref_list = yaml.safe_load(inp)
+                generate_header(pref_list)
+                self.assertEqual(0, 1)
+            except ValueError as e:
+                self.assertEqual(str(e), expected)
+
+
+if __name__ == '__main__':
+    mozunit.main()