Use C++ protection features to enforce abstraction boundaries
UPC++ exposes a number of public classes with deliberately unspecified members used for the implementation that should never be touched by users.
A prominent example is the global_ptr<T>
class template, which currently includes:
public: //private!
#if UPCXX_MANY_KINDS
std::int32_t device_;
#else
static constexpr std::int32_t device_ = -1;
#endif
intrank_t rank_;
T* raw_ptr_;
As public
fields these look very inviting to end users who don't fully read/comprehend our documentation and just access the fields they see (especially in rich IDEs which will show these as public fields, without the comment). This has already happened with several external users and leads to problems when we change the internal fields or they use them incorrectly (because they are undocumented).
C++ has features to prevent exactly this problem. All such fields should really be private
or at least protected
, or (as a last resort) appear in a detail
namespace which we very loudly document as "off-limits".
Comments (7)
-
reporter -
reporter - changed milestone to 2020.9.0 release
Bulk roll-over of unresolved issues to next milestone
-
reporter - changed milestone to 2021.3.0 release
Mass roll-over of open issues to next release milestone
-
Now that PR #326 has been merged, here are the remaining public members I’ve been able to find:
global_ptr<const T, KindSet>::check() persona::backend_state_ persona::cuda_state_ persona::undischarged_n_ persona::active() persona_scope::the_default_dummy_ persona_scope::next_ persona_scope::persona_xor_default_ persona_scope::next_unique_ persona_scope::tls serialization<T>::existence serialization<T>::is_serializable serialization_traits<T>::static_ubound serialization_traits<T>::static_ubound_t future1::kind_type future1::results_type future1::impl_type future1::clref_results_refs_or_vals_type future1::rref_results_refs_or_vals_type future1::result_return_select_type future1::future1(impl_type) future1::then_lazy()
Of these, I think
global_ptr::check()
andfuture1::then_lazy()
would look the most enticing to users. I’ll look into protecting them (likely withUPCXX_INTERNAL_ONLY
forglobal_ptr::check()
and friend declarations forfuture1::then_lazy()
). I’ll also see how much work it would be to protect thepersona
andpersona_scope
members. -
reporter global_ptr::check()
is harmless and provides a sanity check that I actually deliberately added that in the public namespace. I view this as an implementation feature that we could optionally document indocs/implementation-defined.md
, although possibly not something for the formal spec.The
persona
andpersona_scope
internals are alot more dangerous and ideally would be protected somehow (but not if it comes at the cost of additional calls). -
OK, I’ll leave
global_ptr::check()
alone.I will likely look at
future1::then_lazy()
as part of PR #328, since that movesfuture1
into thedetail
namespace.I’ll look at
persona
andpersona_scope
in a separate PR.I don’t think I’ll touch
serialization
orserialization_traits
. -
reporter - changed status to resolved
- Log in to comment
This was discussed in the 2019-12-04 Pagoda meeting.
With regards to the
global_ptr
fields in particular, John explained that these fields are currently declared public because it simplifies usage internal to the runtime. The consensus was the current "trailing underscore" naming convention is insufficient to universally convey to our end users that these fields are private and unsupported (as evidenced by multiple independent support threads on the forum). It was noted that the behavior of theraw_ptr_
field values is especially surprising to users who don't understand our deliberately undocumented shared memory mappings.@john bachan promised to investigate ways to make these fields less "attractive" to end users with smart editors (who usually won't see any comments in the header), and more obviously "danger, these are unsupported and probably don't behave the way you expect".
Possibilities include:
global_ptr::danger_private_raw_ptr
private
and introducing inline accessor methods in thedetail
namespace for use by the runtime