Project

General

Profile

Bug #1679

Do Not Require Default Constructors for Serializable Classes

Added by Nils Deppe 3 months ago. Updated 2 days ago.

Status:
Merged
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
09/18/2017
Due date:
% Done:

0%


Description

I've set the target to 7 because this could be implemented as a breaking change.

Currently Charm++ requires all classes be default constructible. However, with C++11 it is possible to = delete the default constructor, which in many cases is desirable and can help prevent bugs. Charm++ requires the object be created and then a pup member function be called. There are two solutions I can think of:

1) Have classes have a constructor that takes a PUP::er (I can see this being tricky with abstract base classes), and then deserialize the data from it. Some care would need to be taken not to call this constructor when serializing, only when deserializing.

2) Construct classes using a constructor that takes some charm++ defined type, say charm::PupConstructor, and then calling the pup as is done now. This allows people to delete (or make private) the default constructor to prevent unintentional default construction.

I think both are equally easy to implement, especially if one is willing to break code. If you do not want to break code you will need to use something like std::is_constructible (http://en.cppreference.com/w/cpp/types/is_constructible) to check whether the required constructor exists, making this an opt-in feature rather than an enforced idiomatic way of writing Charm++ code.


Related issues

Related to Charm++ - Bug #921: Entry tag [inline] is unable to optimize away most of the overhead In Progress 12/14/2015
Related to Charm++ - Bug #1701: Cannot have non-copyable types in constructor arguments Merged 09/30/2017

History

#1 Updated by Phil Miller 3 months ago

  • Target version changed from 7 (Next Generation Charm++) to 6.9.0
  • Assignee set to Phil Miller
  • Status changed from New to In Progress

We actually ended up implementing something that should satisfy this request for a contract customer who wanted it ASAP: https://charm.cs.illinois.edu/gerrit/#/c/3021/

Using an actual constructor with a tag type argument instead of a separately-named method may be preferable, since the special method ends up requiring also move constructor support.

Note that the patch following the one linked is necessary to fully actually eliminate the requirement of Default Constructible for types used as entry method arguments, since generated code default-constructs objects to unpack-PUP into.

#2 Updated by Phil Miller about 2 months ago

  • Related to Bug #921: Entry tag [inline] is unable to optimize away most of the overhead added

#3 Updated by Phil Miller about 2 months ago

Moved to a constructor with a tag argument approach. We now do not require either of the default or move constructor.

As currently implemented, entry methods can take their arguments by value, rvalue reference, or const lvalue reference. They cannot take them by non-const lvalue reference. Semantically, this is probably right, since the unpacked instances are logically 'expiring' when the call returns, even though they may actually be declared as local variables in the generated calling code. We'll see if the applications can handle this.

Applications that want to be wholly backwards compatible (current/evolved app code builds on charm<6.9) may pay a performance penalty if they want to be able to modify the value in question - they would have to switch to taking their arguments by value, since the older code generation won't let them accept rvalue references. Without breaking backwards compatibility, they could still take const lvalue reference.

#4 Updated by Laxmikant "Sanjay" Kale about 2 months ago

An example program will be useful to have (and/or a fragment) in redmine to illustrate the feature.

#5 Updated by Phil Miller 13 days ago

  • Status changed from In Progress to Implemented

Commits now all pass tests, minor application changes as needed have happened, and documentation is included.

#6 Updated by Phil Miller 2 days ago

  • Status changed from Implemented to Merged

#7 Updated by Phil Miller 2 days ago

  • Related to Bug #1701: Cannot have non-copyable types in constructor arguments added

Also available in: Atom PDF