# fix handling of nested control objects

XMLWordPrintable

#### Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
2
• Team:
Data Release Production

#### Description

Work on the HSC side has revealed some problems with nested control objects being wrapped into config objects. This is a pull request for those changes (along with writing a unit test for some of them).

Some (but not all of these changes) are part of Trac ticket #3163 (https://dev.lsstcorp.org/trac/ticket/3163), which I'll now close as a duplicate.

#### Activity

Hide
Jim Bosch added a comment -

Ready for review on u/jbosch/DM-674 of pex_config:

 pex_config:u/jbosch/DM-674 % git diff --stat master...u/jbosch/DM-674  python/lsst/pex/config/wrap.py | 36 +++++++++++++++++++-----------------  tests/testLib.i | 11 +++++++----  tests/wrap.py | 8 +++++---  3 files changed, 31 insertions(+), 24 deletions(-)

The changeset is pretty small, but it's got some deep pex_config internals (more precisely, it adds pre-existing pex_config internals that improve history readability to the control-object-wrapping code).

Show
Jim Bosch added a comment - Ready for review on u/jbosch/ DM-674 of pex_config: pex_config:u/jbosch/DM-674 % git diff --stat master...u/jbosch/DM-674 python/lsst/pex/config/wrap.py | 36 +++++++++++++++++++----------------- tests/testLib.i | 11 +++++++---- tests/wrap.py | 8 +++++--- 3 files changed, 31 insertions(+), 24 deletions(-) The changeset is pretty small, but it's got some deep pex_config internals (more precisely, it adds pre-existing pex_config internals that improve history readability to the control-object-wrapping code).
Hide
Russell Owen added a comment -

Overall this looks like good.

It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.)

wrap.py:

• You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining.
• In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11?

testLib.i and wrap.py:

• I suggest a test for boost::int64_t
Show
Russell Owen added a comment - Overall this looks like good. It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.) wrap.py: You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining. In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11? testLib.i and wrap.py: I suggest a test for boost::int64_t
Hide
Jim Bosch added a comment -

It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.)

It's subtle: the real bug that kicked all of this off was that previously, when an outer control object changed the value of an inner control object in its constructor (the moral equivalent of changing a sub-config value in the outer config's setDefaults()), this was not propagated to the defaults in the generated config object. Using the internal arguments and delegating to update() allowed me to do some cleanup while fixing that bug.

• You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining.

It only assumes that Python's int is large enough to hold 64-bit values, which I believe has been true even on 32-bit platforms since Python 2.4. I've added a comment explaining that we're not looking for binary under-the-hood compatibility here, just the ability to round-trip through the usual Swig converters.

• In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11?

Oh, probably, but it'll be trickier since it has an extra template parameter too, so I don't plan to add until we have a use case for it. In any case, I've given up holding my breath on C++11 in the stack for fear of asphyxiation

• I suggest a test for boost::int64_t

Done.

Show
Jim Bosch added a comment - It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.) It's subtle: the real bug that kicked all of this off was that previously, when an outer control object changed the value of an inner control object in its constructor (the moral equivalent of changing a sub-config value in the outer config's setDefaults()), this was not propagated to the defaults in the generated config object. Using the internal arguments and delegating to update() allowed me to do some cleanup while fixing that bug. You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining. It only assumes that Python's int is large enough to hold 64-bit values, which I believe has been true even on 32-bit platforms since Python 2.4. I've added a comment explaining that we're not looking for binary under-the-hood compatibility here, just the ability to round-trip through the usual Swig converters. In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11? Oh, probably, but it'll be trickier since it has an extra template parameter too, so I don't plan to add until we have a use case for it. In any case, I've given up holding my breath on C++11 in the stack for fear of asphyxiation I suggest a test for boost::int64_t Done.

#### People

Assignee:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
Russell Owen
Watchers:
Jim Bosch, Russell Owen