This is an archive of the discontinued LLVM Phabricator instance.

Fix/unify SBType comparison
ClosedPublic

Authored by labath on Mar 11 2019, 9:34 AM.

Details

Summary

In my next step at cleaning up modify-python-lldb.py, I started focusing
on equality comparison. To my surprise, I found out that both python and
c++ versions of the SBType class implement equality comparison, but each
one does it differently. While the python version was implemented in
terms of type name equality, the C++ one used a deep comparison on the
underlying objects.

Removing the python version caused one test to fail (TestTypeList). This
happened because the c++ version of operator== boiled down to
TypePair::operator==, which contains two items: the compiler_type and
type_sp. In this case, the compiler_type was identical, but one of the
objects had the type_sp field unset.

I tried fixing the code so that both objects keep their type_sp member,
but it wasn't easy, because there are so many operations which just work
with the CompilerType types, and so any operation on the SBType (the
test in question was doing GetPointeeType on the type of one variable
and expecting it to match the type of another variable), cause that
second member to be lost.

So instead, I tried to relax the equality comparison on the TypePair
class. Now, this class ignores the type_sp for the purposes of
comparison, and uses the CompilerType only. This seems reasonable, as
each TypeSP is able to convert itself to a CompilerType, though I don't
understand the full implications of this (e.g., is there any case where
two different TypeSPs might end up returning different CompilerTypes).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Mar 11 2019, 9:34 AM
clayborg accepted this revision.Mar 11 2019, 10:06 AM
This revision is now accepted and ready to land.Mar 11 2019, 10:06 AM

Interesting find. Thanks!

SBType storing a pair of CompilerType and TypeSP seems like a very confusing interface and like it will be very easy to misuse or violate assumptions (perhaps even assumptions that nobody knows exists). Why exactly is this necessary?

As far as I can tell, SBType uses TypeImpl, which supports having a CompilerType, a TypeSP, or both, but I cannot find any client of TypeImpl which actually depends on the TypeSP being set. Perhaps we can just delete support for TypeSP from TypeImpl entirely.

If no one is depending on type_sp, then ok to delete. Should be easy to test by removing it and seeing what breaks (if anything).

Thanks. I'm going to commit this, and then see whether it is possible to remove the type_sp member.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2019, 6:46 AM