JDevlieghere added a comment.
I like the new approach, it's much easier to follow. I also like the wrapper
class to abstract over the common operations. It just seems like there are some
remnants of the canonical wp approach that we no longer need.
================
Comment at: lldb/include/lldb/Symbol/CompilerType.h:51
+ class ManagedTypeSystem {
+ lldb::TypeSystemSP m_typesystem_sp = nullptr;
+
----------------
nit: `lldb::TypeSystemSP m_typesystem_sp = {};` or even `lldb::TypeSystemSP
m_typesystem_sp`
================
Comment at: lldb/include/lldb/Symbol/CompilerType.h:58-60
+ if (!m_typesystem_sp)
+ return nullptr;
+ return llvm::dyn_cast_or_null<TypeSystemType>(m_typesystem_sp.get());
----------------
If it's a `dyn_cast_or_null` why not do the `m_typesystem_sp` check, won't
`dyn_cast_or_null` repeat that for you anyway?
================
Comment at: lldb/include/lldb/Symbol/TypeSystem.h:88-91
+ /// Returns a weak pointer to this type system. For use in
+ /// CompilerType and other places that need to hold on to a
+ /// TypeSystem.
+ lldb::TypeSystemWP &GetAsWeakPtr();
----------------
Now that the weak pointer is no longer "canonical", there's no reason that this
should be a reference anymore, right?
================
Comment at: lldb/include/lldb/Symbol/TypeSystem.h:517-522
+ /// Only to be used by TypeSystemMap and various unit tests.
+ void SetCanonicalWeakPtr(lldb::TypeSystemWP wp) { m_canonical_wp = wp; }
protected:
+ lldb::TypeSystemWP m_canonical_wp;
SymbolFile *m_sym_file = nullptr;
};
----------------
Do we still need this? I assume we can now just return the weak pointer
directly (through shared_from_this)?
================
Comment at: lldb/include/lldb/Symbol/TypeSystem.h:550-553
+ /// A canoncial pointer to a weak pointer to the type system. This
+ /// allows CompilerType to store the weak pointer in a single
+ /// pointer-sized field. This field is never a nullptr.
+ lldb::TypeSystemWP *wp_ptr;
----------------
No longer necessary?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136650/new/
https://reviews.llvm.org/D136650
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
