cpp_const: Add ability to impose C++ const semantics in Python.

[EricCousineau-TRI]

Copying from #7841:

This implements functionality to enable a potential solution for #7793, and to provide workarounds for pybind/pybind11#1241 (callbacks and virtual function overrides - #7838).

C++ Pybind Code - cpp_const_pybind_test_py.cc
Python Code, using C++ - cpp_const_pybind_test.py

This imports a portion of wrapt with slight modifications (shown in the commits), and enables the pure-Python portion of the feature.

Please note that this PR exceeds the nominal line count due to third-party code; however, excluding the third party code, the line count is 421 insertions:

$ git diff --shortstat HEAD~ -- bindings/
 3 files changed, 421 insertions(+)

---

This change is 

[EricCousineau-TRI]

+@soonho-tri for feature review, if you have time, please.
+@jwnimmer-tri for platform review on Monday, please. (Let me know if you want this code incorporated in a different fashion.)

---

Review status: 0 of 8 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

(Let me know if you want this code incorporated in a different fashion.)

You mean the //third_party/...? Seems fine to vendor it, since you want to make edits. It looks like our pydrake install rule installs pydrake code that depends on it though, so you'll need to make sure the installed bindings work, and that the new LICENSE file gets installed.

[soonho-tri]

Reviewed 4 of 5 files at r1, 2 of 4 files at r2.
Review status: 6 of 8 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 4 of 5 files at r1, 2 of 4 files at r2.
Review status: 6 of 8 files reviewed at latest revision, 3 unresolved discussions.

---

a discussion (no related file):
BTW It would probably be cleaner if the first commit (importing from upstream) had only the files actually from upstream + the README; as of now it also has the init and BUILD files. A simple "git blame" ends up being misleading right now, though if someone consulted the README they could figure it out.

---

bindings/pydrake/util/BUILD.bazel, line 127 at r2 (raw file):

UPSTREAM_INSTALLS = [
    "//third_party/wrapt:install",
]

If I recall correctly, our convention is to have something more like //tools/workspace:BUILD.bazel handle the third-party license installs. Another option would be to have //third_party:BUILD.bazel have the install rule. In any case, burying it here is surprising enough to be defective.

---

third_party/wrapt/BUILD.bazel, line 23 at r2 (raw file):

    name = "install",
    targets = [":wrapt"],
    py_dest = "lib/python2.7/dist-packages/wrapt",

TBD Does this go into the right place?

---

Comments from Reviewable

[jwnimmer-tri]

third_party/wrapt/BUILD.bazel, line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

TBD Does this go into the right place?

(Sorry, this was a note to myself. I'll follow-up later.)

---

Comments from Reviewable

[EricCousineau-TRI]

Per f2f with @jwnimmer-tri, we looked into using Ubuntu Debian packages / Mac PIP packages. However, the Ubuntu package is tagged with 1.8.0, and looking at the diff of wrapt between 1.8.0 and 1.10.11, it looks like there was bug in which __mod__ was implemented using a ^ b rather than a % b.

Given this, will use a fork for the time being. I will add a TODO to drop the fork once later versions of python-wrapt are available on Ubuntu.

---

Review status: 6 of 8 files reviewed at latest revision, 3 unresolved discussions.

---

third_party/wrapt/BUILD.bazel, line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

TBD Does this go into the right place?

OK Actually, I'm realizing that I want this to go to drake.third_party.wrapt - I don't want our changes to conflict with other people's usages.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 4 files at r2.
Review status: 7 of 8 files reviewed at latest revision, 19 unresolved discussions.

---

bindings/pydrake/util/cpp_const.py, line 2 at r2 (raw file):

"""
@file

BTW Is @file a thing python community and/or pydoc knows about? I would have thought that the top level __doc__ was already understood to be file-wide overview?

---

bindings/pydrake/util/cpp_const.py, line 11 at r2 (raw file):

# N.B. There should *always* be an option for disabling this for performance
# reasons!

I am not sure what I, as a reader, am supposed to take away from reading this comment.

---

bindings/pydrake/util/cpp_const.py, line 18 at r2 (raw file):

class ConstError(TypeError):

Probably requires pydoc class overview per styleguide?

---

bindings/pydrake/util/cpp_const.py, line 27 at r2 (raw file):

        self._cls = cls
        self._owned_properties = set(owned_properties or set())
        self.mutable_methods = set(mutable_methods or set())

FYI Access to this isn't called in a tight loop, so consider hiding it behind an accessor for privacy? Alternatively -- unclear why some members are private and some are public.

---

bindings/pydrake/util/cpp_const.py, line 50 at r2 (raw file):

        # Limitation: This would not handle overloads (e.g.
        # `const T& get() const` and `T& get()`). However, C++ should decorate
        # the methods such that we should not need to mark them.

Unclear what decorate means here. What specific mechanism?

---

bindings/pydrake/util/cpp_const.py, line 82 at r2 (raw file):

_const_metas = _ConstClassMetaMap()
# Register common mutators.
# N.B. These methods actually have to be overridde in `Const`, since neither

BTW Typo ridden.

---

bindings/pydrake/util/cpp_const.py, line 98 at r2 (raw file):

    "__imod__",
    "__ipow__",
    "__ilshift__",

Missing __irshift__.

---

bindings/pydrake/util/cpp_const.py, line 111 at r2 (raw file):

    'insert',
    'pop',
    'remove',

Missing reverse.

---

bindings/pydrake/util/cpp_const.py, line 117 at r2 (raw file):

    'clear',
    'setdefault',
    })

Missing:
__missing__
pop
popitem
... of if they are already handled (because they delegate to another method) please comment as such.

---

bindings/pydrake/util/cpp_const.py, line 150 at r2 (raw file):

    def __str__(self):
        out = self.__wrapped__.__str__()
        if (len(out) >= 2 and len(out) < 200 and

I couldn't figure out the reason for the 200 limit.

---

bindings/pydrake/util/cpp_const.py, line 159 at r2 (raw file):

def _install_mutable_error_methods():
    # Automatically rewrite any mutable methods for `object` to deny access
    # upon calling.

It took me a while before I realized that this was rebinding _Const's inherited methods from object -- specifically, that "any mutable methods for object" meant "on the _Const class". (I thought we were intercepting all object methods everywhere!)

---

bindings/pydrake/util/cpp_const.py, line 194 at r2 (raw file):

def to_const(obj):
    """Converts an object to a const proxy. Does not proxy immutable / literal
    types. """

BTW Meta No whitespace between . ", throughout.

---

bindings/pydrake/util/cpp_const.py, line 194 at r2 (raw file):

def to_const(obj):
    """Converts an object to a const proxy. Does not proxy immutable / literal
    types. """

FYI I assume basically any kind of object could be passed in here? If yes, great. If not and there's things I shouldn't pass in here, then please document then.

---

bindings/pydrake/util/cpp_const.py, line 232 at r2 (raw file):

    direct type.
    """
    if isinstance(obj, _Const):

Rule-of-3 duplication of this check (just calling the existing helper?).

---

bindings/pydrake/util/cpp_const.py, line 244 at r2 (raw file):

def mutable_method(func):

mutable_method is a bit of a confusing name, because its missing a verb.

Ditto for const_meta below.

---

bindings/pydrake/util/cpp_const.py, line 257 at r2 (raw file):

def const_meta(owned_properties=None, mutable_methods=None):
    """Decorates a class with const-proxy metadata. """

Actually, it returns a functor that does so?

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 5 files at r2, 1 of 16 files at r3.
Review status: 2 of 12 files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

---

bindings/pydrake/util/cpp_const.py, line 26 at r3 (raw file):

    def __init__(self, cls, owned_properties=None, mutable_methods=None):
        self._cls = cls
        self._owned_properties = set(owned_properties or set())

FYI Should it be an error here if owned_properties mentions a field that doesn't exist on cls? Otherwise it seems to me like typos in using-code might go unnoticed?

---

bindings/pydrake/util/test/cpp_const_test.py, line 4 at r3 (raw file):

import unittest

import pydrake.util.cpp_const as m

FYI I am not loving m as a global variable name; usually 1-letter variables would be loop iterators, or at least locals. I think its borderline enough that I'm not going to hard defect it, but consider sticking with cpp_const for clarity, or perhaps as mut # Module under test. to split the difference on line wrap vs readability. (Or is m a pythonic habit that I just haven't seen yet?)

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 0 of 12 files reviewed at latest revision, 21 unresolved discussions.

---

bindings/pydrake/util/BUILD.bazel, line 127 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I recall correctly, our convention is to have something more like //tools/workspace:BUILD.bazel handle the third-party license installs. Another option would be to have //third_party:BUILD.bazel have the install rule. In any case, burying it here is surprising enough to be defective.

Done.

---

bindings/pydrake/util/cpp_const.py, line 2 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Is @file a thing python community and/or pydoc knows about? I would have thought that the top level __doc__ was already understood to be file-wide overview?

Done.

---

bindings/pydrake/util/cpp_const.py, line 11 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not sure what I, as a reader, am supposed to take away from reading this comment.

Done.

---

bindings/pydrake/util/cpp_const.py, line 27 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Access to this isn't called in a tight loop, so consider hiding it behind an accessor for privacy? Alternatively -- unclear why some members are private and some are public.

OK mutable_methods is public because it is used by _install_object_mutable_methods, but it's the only property used.

---

bindings/pydrake/util/cpp_const.py, line 50 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear what decorate means here. What specific mechanism?

Done.

---

bindings/pydrake/util/cpp_const.py, line 82 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Typo ridden.

Done.

---

bindings/pydrake/util/cpp_const.py, line 98 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Missing __irshift__.

Done.

---

bindings/pydrake/util/cpp_const.py, line 111 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Missing reverse.

Done.

---

bindings/pydrake/util/cpp_const.py, line 117 at r2 (raw file):
Done.

__missing__ is not intrinsically mutable:

class Dict(dict):
    def __missing__(self, key):
        return key
...
>>> d = Dict(a=1)
>>> d["b"]
'b'
>>> d
{'a': 1}
>>> 

---

bindings/pydrake/util/cpp_const.py, line 150 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I couldn't figure out the reason for the 200 limit.

Done. Removed limit, and ensured that this only applies to repr.

---

bindings/pydrake/util/cpp_const.py, line 159 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It took me a while before I realized that this was rebinding _Const's inherited methods from object -- specifically, that "any mutable methods for object" meant "on the _Const class". (I thought we were intercepting all object methods everywhere!)

Done.

---

bindings/pydrake/util/cpp_const.py, line 194 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I assume basically any kind of object could be passed in here? If yes, great. If not and there's things I shouldn't pass in here, then please document then.

Done.

---

bindings/pydrake/util/cpp_const.py, line 194 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Meta No whitespace between . ", throughout.

Done.

---

bindings/pydrake/util/cpp_const.py, line 232 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Rule-of-3 duplication of this check (just calling the existing helper?).

Done.

---

bindings/pydrake/util/cpp_const.py, line 244 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

mutable_method is a bit of a confusing name, because its missing a verb.

Ditto for const_meta below.

OK I'd prefer to keep it in-sync with other Python decorators, like staticmethod or property.

Changed const_meta to const_decorated, though, as const_class nor mutable_class make much of any sense.

---

bindings/pydrake/util/cpp_const.py, line 257 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Actually, it returns a functor that does so?

Done.

---

bindings/pydrake/util/cpp_const.py, line 26 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Should it be an error here if owned_properties mentions a field that doesn't exist on cls? Otherwise it seems to me like typos in using-code might go unnoticed?

OK I'd prefer not to restrict this. Most classes that I've seen do not indicate their members until __init__ is called, and the properties are added dynamically (and not really as properties per se, but entries in the instance's __dict__.

---

bindings/pydrake/util/test/cpp_const_test.py, line 4 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I am not loving m as a global variable name; usually 1-letter variables would be loop iterators, or at least locals. I think its borderline enough that I'm not going to hard defect it, but consider sticking with cpp_const for clarity, or perhaps as mut # Module under test. to split the difference on line wrap vs readability. (Or is m a pythonic habit that I just haven't seen yet?)

Done. It's a convention used in pybind11, but changed it to cpp_const for clarity.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 0 of 12 files reviewed at latest revision, 14 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW It would probably be cleaner if the first commit (importing from upstream) had only the files actually from upstream + the README; as of now it also has the init and BUILD files. A simple "git blame" ends up being misleading right now, though if someone consulted the README they could figure it out.

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 0 of 12 files reviewed at latest revision, 13 unresolved discussions.

---

bindings/pydrake/util/cpp_const.py, line 18 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Probably requires pydoc class overview per styleguide?

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 13 of 16 files at r3, 2 of 2 files at r4.
Review status: 10 of 12 files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/util/cpp_const.py, line 117 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

Done.

__missing__ is not intrinsically mutable:

class Dict(dict):
    def __missing__(self, key):
        return key
...
>>> d = Dict(a=1)
>>> d["b"]
'b'
>>> d
{'a': 1}
>>> 

OK Ah, yes! I misread the python docs.

---

bindings/pydrake/util/cpp_const.py, line 244 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

OK I'd prefer to keep it in-sync with other Python decorators, like staticmethod or property.

Changed const_meta to const_decorated, though, as const_class nor mutable_class make much of any sense.

Aha, I didn't realize (while reading) that these are intended to be used as Python decorators. In that case, the names make perfect sense, but the documentation is unclear.

---

bindings/pydrake/util/cpp_const.py, line 26 at r3 (raw file):

Previously, EricCousineau-TRI wrote…

OK I'd prefer not to restrict this. Most classes that I've seen do not indicate their members until __init__ is called, and the properties are added dynamically (and not really as properties per se, but entries in the instance's __dict__.

OK Good point.

---

bindings/pydrake/util/cpp_const.py, line 232 at r4 (raw file):

    WARNING: Do NOT use this for branching in production code unless
    const-proxying is guaranteed to be enabled or the branching is designed
    not to fail in this case.

I guess we can't name it _is_const because we want to unit test it?

The warning seems dire enough (and correctly so) that the method name should perhaps reflect that? Or we could promise to fail-fast here if const checking got compiled out or disabled?

---

Comments from Reviewable

[jwnimmer-tri]

LGTM Modulo future changes from @soonho-tri review.

---

Reviewed 2 of 16 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

bindings/pydrake/BUILD.bazel, line 140 at r4 (raw file):

    "//drake/bindings/pydrake/third_party",
    "//drake/bindings/pydrake/util",
])

FYI Is the name PYBIND_LIBRARIES still applicable, with this new item added?

---

bindings/pydrake/third_party/forward_files.bzl, line 6 at r4 (raw file):

        srcs,
        strip_prefix,
        dest_prefix,

BTW I believe its skylark style to use kwargs syntax (= None) for all non-name arguments.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
FYI I've confirmed that the installed files end up in good places ...

lib/python2.7/site-packages/pydrake/third_party/wrapt
lib/python2.7/site-packages/pydrake/third_party/wrapt/__init__.py
lib/python2.7/site-packages/pydrake/third_party/wrapt/wrappers.py
share/doc/wrapt
share/doc/wrapt/LICENSE

... and that I can import pydrake.util.cpp_const from the installed image.

---

Comments from Reviewable

[soonho-tri]

Checkpoint.

---

Reviewed 15 of 16 files at r3, 1 of 2 files at r4.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

bindings/pydrake/util/cpp_const.py, line 3 at r3 (raw file):

"""
@file
Enables a semi-transparent layer of C++ const-honoring Python proxies.

Can you explain why this is "semi-" transparent?

---

bindings/pydrake/util/cpp_const.py, line 41 at r4 (raw file):

    def is_owned_property(self, name):
        # Determines if a property is owned, and if the returned value should

Is "a property is owned" the same as "the returned value should be const"?

---

bindings/pydrake/util/cpp_const.py, line 85 at r4 (raw file):

# N.B. All methods registered will respect inheritance.
# N.B. For `object`, see `_install_object_mutable_methods`.
_const_metas.emplace(object, mutable_methods={

Is it a list of built-in methods available in Python object? As a reviewer, I'd like to check if this list is sound and complete but I do not have a clue how to do that (i.e. dir(object) gives a different lists, not a super set of it, for instance). Ditto for list and dict below.

---

bindings/pydrake/util/cpp_const.py, line 183 at r4 (raw file):

    # Detects if a type is a immutable (or literal) type.
    literal_types = [int, float, str, unicode, tuple, type(None)]
    if type(obj) in literal_types:

BTW, this can be just return type(obj) in literal_types?

---

bindings/pydrake/util/cpp_const.py, line 232 at r4 (raw file):

    WARNING: Do NOT use this for branching in production code unless
    const-proxying is guaranteed to be enabled or the branching is designed
    not to fail in this case.

FYI, I don't understand "the branching is designed not to fail in this case" part. Could you elaborate?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 7 of 17 files reviewed at latest revision, 10 unresolved discussions.

---

bindings/pydrake/BUILD.bazel, line 140 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Is the name PYBIND_LIBRARIES still applicable, with this new item added?

Done. Made a rename across pydrake, can revert that commit if it's scope creep.

---

bindings/pydrake/third_party/forward_files.bzl, line 6 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I believe its skylark style to use kwargs syntax (= None) for all non-name arguments.

Done.

---

bindings/pydrake/util/cpp_const.py, line 244 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Aha, I didn't realize (while reading) that these are intended to be used as Python decorators. In that case, the names make perfect sense, but the documentation is unclear.

Done.

---

bindings/pydrake/util/cpp_const.py, line 3 at r3 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Can you explain why this is "semi-" transparent?

Done.

---

bindings/pydrake/util/cpp_const.py, line 41 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Is "a property is owned" the same as "the returned value should be const"?

Done.

---

bindings/pydrake/util/cpp_const.py, line 85 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Is it a list of built-in methods available in Python object? As a reviewer, I'd like to check if this list is sound and complete but I do not have a clue how to do that (i.e. dir(object) gives a different lists, not a super set of it, for instance). Ditto for list and dict below.

Done.

---

bindings/pydrake/util/cpp_const.py, line 183 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, this can be just return type(obj) in literal_types?

Done.

---

bindings/pydrake/util/cpp_const.py, line 232 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I guess we can't name it _is_const because we want to unit test it?

The warning seems dire enough (and correctly so) that the method name should perhaps reflect that? Or we could promise to fail-fast here if const checking got compiled out or disabled?

Done.
Yuppers, and I'd prefer not to make the name confined to just testing, as it is used in to_const and to_mutable. I've added the *_test suffix to (faintly) try to make people scratch their heads.

---

bindings/pydrake/util/cpp_const.py, line 232 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

FYI, I don't understand "the branching is designed not to fail in this case" part. Could you elaborate?

Done.

---

Comments from Reviewable

[jwnimmer-tri]

---

Reviewed 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/third_party/forward_files.bzl, line 6 at r4 (raw file):

Previously, EricCousineau-TRI wrote…

Done.

BTW also srcs?

---

tools/skylark/pybind.bzl, line 156 at r5 (raw file):

def _get_install(target):
    # Gets the install target for a Python target that have a sibling install

BTW typo has

---

Comments from Reviewable

[soonho-tri]

---

Reviewed 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/util/cpp_const.py, line 166 at r5 (raw file):

    def __dict_custom__(self):
        # Prevent direct mutation of dictionary.

BTW, I was wondering if it's safe to use double-underscore (__) prefix for a non-system method such as this one. (This is really a question for my Python education).

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 14 of 17 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/util/cpp_const.py, line 166 at r5 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, I was wondering if it's safe to use double-underscore (__) prefix for a non-system method such as this one. (This is really a question for my Python education).

Done. Added comments in wrapt/wrappers.py. Does that help?

---

tools/skylark/pybind.bzl, line 156 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW typo has

Done.

---

Comments from Reviewable

[soonho-tri]

Reviewed 1 of 16 files at r3, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

bindings/pydrake/util/cpp_const.py, line 166 at r5 (raw file):

Previously, EricCousineau-TRI wrote…

Done. Added comments in wrapt/wrappers.py. Does that help?

SGTM!

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 16 files at r3, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[soonho-tri]

Reviewed 4 of 4 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 4 of 4 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable