Skip to content

Commit

Permalink
pxrTf: Make sure to use correct ordering of base types when declaring…
Browse files Browse the repository at this point in the history 
… TfTypes.

Previously, the order of bases was defined on the go as types were (repeatedly)
declared: A common scenario was that Plug would declare a type using a subset
of the bases and then C++ code would supply all bases.   However,
only additional bases would be added to a TfType's list of bases.  This resulted
in a mangled order of bases.

Many behaviors in the system are based on TfType's ordering of bases, mostly
via TfType::GetAllAncestorTypes().

What made things worse is that the order in which TfType's are declared is
not stable: Sometimes Plug comes first, sometimes the types declared in the
C++ code.

This change makes it so that the ordering will be stable and adds more testing
and error messages if clients attempt to change the base order.

(Internal change: 1948578)
  • Loading branch information
@florihupf @pixar-oss
florihupf authored and pixar-oss committed Mar 18, 2019
1 parent 4ac3277 commit f398984
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 44 deletions.
43 changes: 43 additions & 0 deletions pxr/base/lib/tf/testenv/type.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,49 @@ Test_TfType()
TfType found = tConcrete.FindDerivedByName("SomeClassB");
TF_AXIOM(found == tClassA);

////////////////////////////////////////////////////////////////////////
// Test that bases are registered with the correct order and that errors
// are posted as needed.

TfType someBaseA = TfType::Declare("SomeBaseA");
TfType someBaseB = TfType::Declare("SomeBaseB");
TF_AXIOM(someBaseA && someBaseB);

{
using TypeVector = std::vector<TfType>;

// Define SomeDerivedClass with base SomeBaseB: No error expected.
TfErrorMark m;
TfType t = TfType::Declare("SomeDerivedClass", { someBaseB });
TF_AXIOM(t.GetBaseTypes() == TypeVector({ someBaseB }));
TF_AXIOM(m.IsClean());

// Now redeclare with more bases and an order change: No error expected,
// but someBaseA needs to be first base now.
t = TfType::Declare("SomeDerivedClass", { someBaseA, someBaseB });
TF_AXIOM(t.GetBaseTypes() == TypeVector({ someBaseA, someBaseB }));
TF_AXIOM(m.IsClean());

// Redefine with flipped order: error expected.
TfType::Declare("SomeDerivedClass", { someBaseB, someBaseA });
TF_AXIOM(!m.IsClean() && m.begin()->GetCommentary() ==
"Specified base type order differs for SomeDerivedClass: had "
"(SomeBaseA, SomeBaseB), now (SomeBaseB, SomeBaseA). If this is "
"a type declared in a plugin, check that the plugin metadata is "
"correct.");
TF_AXIOM(m.Clear());

// Redefine with one base missing: error expected.
TfType::Declare("SomeDerivedClass", { someBaseA });
TF_AXIOM(!m.IsClean() && m.begin()->GetCommentary() ==
"TfType 'SomeDerivedClass' was previously declared to have "
"'SomeBaseB' as a base, but a subsequent declaration does not "
"include this as a base. The newly given bases were: (SomeBaseA). "
"If this is a type declared in a plugin, check that the plugin "
"metadata is correct.");
TF_AXIOM(m.Clear());
}

return true;
}

Expand Down
132 changes: 90 additions & 42 deletions pxr/base/lib/tf/type.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,11 +836,12 @@ TfType::Declare(const string &typeName,
}

// Update base types.
vector<TfType> &bases = t._info->baseTypes;
const vector<TfType> &haveBases = t._info->baseTypes;

// If this type already directly inherits from root, then
// prohibit adding any new bases.
if (!newBases.empty() && bases == vector<TfType>(1, GetRoot())) {
if (!newBases.empty() &&
haveBases.size() == 1 && haveBases.front() == GetRoot()) {
errorsToEmit.push_back(
TfStringPrintf("Type '%s' has been declared to have 0 bases, "
"and therefore inherits directly from the root "
Expand All @@ -850,39 +851,13 @@ TfType::Declare(const string &typeName,
}

if (newBases.empty()) {
if (bases.empty()) {
if (haveBases.empty()) {
// If we don't have any bases yet, add the root type.
t._AddBase(GetRoot());
t._AddBases(TypeVector(1, GetRoot()), &errorsToEmit);
}
} else {
// Otherwise, add the new bases.

// First, check that all previously-declared bases are included.
TF_FOR_ALL(it, bases) {
if (std::find(newBases.begin(), newBases.end(), *it) ==
newBases.end())
{
string newBasesStr;
TF_FOR_ALL(it2, newBases)
newBasesStr += it2->GetTypeName() + " ";

errorsToEmit.push_back(
TfStringPrintf("TfType '%s' was previously declared to "
"have '%s' as a base, but subsequent "
"declaration does not include this as a "
"base. The newly given bases were: "
"(%s). If this is a type declared in a "
"plugin, check that the plugin metadata "
"is correct.",
t.GetTypeName().c_str(),
it->GetTypeName().c_str(),
newBasesStr.c_str()));
}
}

TF_FOR_ALL(it, newBases)
t._AddBase(*it);

t._AddBases(newBases, &errorsToEmit);
}

if (definitionCallback) {
Expand Down Expand Up @@ -955,20 +930,93 @@ TfType::_DefineCppType(const std::type_info & typeInfo,
}

void
TfType::_AddBase(TfType base) const
TfType::_AddBases(
const TypeVector &newBases, vector<string> *errorsToEmit) const
{
// Callers must hold _info write lock.
if (base.IsUnknown()) {
TF_CODING_ERROR("Specified base type is unknown, skipping");
return;
TypeVector &haveBases = _info->baseTypes;

// Also we check that all previously-declared bases are included and make
// sure that a subsequent registration of base types doesn't change the
// order.
TypeVector::const_iterator lastNewBaseIter = newBases.begin();

for(const TfType &haveBase : haveBases) {

const TypeVector::const_iterator newIter =
std::find(newBases.begin(), newBases.end(), haveBase);

// Repeated base declaration must include all previous bases.
if (newIter == newBases.end()) {

string newBasesStr;
for(const TfType &newBase : newBases) {
newBasesStr += newBasesStr.empty() ? "" : ", ";
newBasesStr += newBase.GetTypeName();
}

errorsToEmit->push_back(TfStringPrintf(
"TfType '%s' was previously declared to have '%s' as a base, "
"but a subsequent declaration does not include this as a base. "
"The newly given bases were: (%s). If this is a type declared "
"in a plugin, check that the plugin metadata is correct.",
GetTypeName().c_str(),
haveBase.GetTypeName().c_str(),
newBasesStr.c_str()));

} else {

// Make sure the new bases are also ordered strictly monotonically
// increasing so that it matches the old order.

if (lastNewBaseIter > newIter) {

std::string haveStr, newStr;
for(const TfType &t : haveBases) {
haveStr += haveStr.empty() ? "" : ", ";
haveStr += t.GetTypeName();
}
for(const TfType &t : newBases) {
newStr += newStr.empty() ? "" : ", ";
newStr += t.GetTypeName();
}
errorsToEmit->push_back(TfStringPrintf(
"Specified base type order differs for %s: had (%s), now "
"(%s). If this is a type declared in a plugin, check that "
"the plugin metadata is correct.",
GetTypeName().c_str(), haveStr.c_str(), newStr.c_str()));
}

lastNewBaseIter = newIter;
}
}
vector<TfType> & bases = _info->baseTypes;
if (std::find(bases.begin(), bases.end(), base) == bases.end()) {
// Add the new base.
bases.push_back(base);
// Tell the new base that it has a new derived type.
ScopedLock baseLock(base._info->mutex, /*write=*/true);
base._info->derivedTypes.push_back(*this);

// If we now have more base types, we use the new, longer vector of base
// types to define the order. Note that we don't need to register any
// derived types in that case, because we just ensured we only expanding
// the set of bases.

if (newBases.size() > haveBases.size()) {

for(const TfType &newBase : newBases) {
if (newBase.IsUnknown()) {
errorsToEmit->push_back(
"Specified base type is unknown, skipping.");
continue;
}
if (std::find(haveBases.begin(), haveBases.end(), newBase) ==
haveBases.end()) {

// Tell the new base that it has a new derived type.
ScopedLock baseLock(newBase._info->mutex, /*write=*/true);
newBase._info->derivedTypes.push_back(*this);
}
}

// Fully replace the list of existing bases if needed. This is so that
// we set the order even if we register bases for a type (partially)
// multiple times.
_info->baseTypes = newBases;
}
}

Expand Down
6 changes: 4 additions & 2 deletions pxr/base/lib/tf/type.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,8 +683,10 @@ class TfType : boost::totally_ordered<TfType>
// Construct a TfType with the given _TypeInfo.
explicit TfType(_TypeInfo *info) : _info(info) {}

// Add a base type, and link as a derived type of that base.
void _AddBase( TfType base ) const;
// Adds base type(s), and link as a derived type of that bases.
void _AddBases(
const std::vector<TfType> &bases,
std::vector<std::string> *errorToEmit) const;

// Add the given function for casting to/from the given baseType.
TF_API
Expand Down

0 comments on commit f398984

Please sign in to comment.