[api] Promote Internal::Parameter to Halide::Parameter #7829
Conversation
…ation API refactoring)
There was a problem hiding this comment.
From a quick skim, it looks like this change is simpy moving it into the right namespace -- we also need to take a close look at the methods that Parameter exposes (including the ctors); IMHO we want to prune it down to the bare minimum we think are necessary (enforcing via friend access if necessary), because otherwise we'll end up with a lot of weird usage we didn't expect (https://www.hyrumslaw.com/).
Once that is done, we will also need to provide a proper Python wrapper for the resulting API; the current Python wrapper (in PyParam.cpp) is deliberately a bare-minimum wrapper that was put in place only for some internal use, but with Parameter become a public Halide object, we need to ensure the Python wrapper provides the same.
Agreed. I intentionally left obvious internal methods (like |
protected. Make Pipeline and Serializer protected friend classes.
|
I made the raw_buffer(), scalar_address(), and scalar_raw_value() access methods protected, and added Pipeline and Serializer as protected friends. I believe these are the only potentially controversial class methods which expose addresses and internal values. |
There was a problem hiding this comment.
LGTM but I think we need to also flush out the Python wrapper for Parameter -- as mentioned elsewhere, the Python wrapper is currently just a minimal stub for internal use, but with this change, we need to promote it to full public status, and provide all the public methods in Python as well as C++.
Should we update the python bindings in this PR or open a new one? |
Unless there is an urgent reason we need to land this quickly, I think it would be better to work out the Python bindings in this PR as well; I've found it a useful exercise in the past to winnow out the calls that are actually necessary for the public API from the ones that aren't. |
Remove old stub internal interface from PyParam.
…de into dg/api_make_parameter_public
|
LGTM |
* Promote Internal::Parameter to Halide::Parameter (to support Serialization API refactoring) * Make raw_buffer(), scalar_address(), and scalar_raw_value() methods protected. Make Pipeline and Serializer protected friend classes. * Add Parameter public interface to python bindings. Remove old stub internal interface from PyParam. * Remove blank line at start of function --------- Co-authored-by: Derek Gerstmann <[email protected]> Co-authored-by: Steven Johnson <[email protected]>
As per the discussion regarding refactoring the Serialization API:
#7760