[Lldb-commits] [PATCH] D136650: Make CompilerType safe [Was: Add a check for TypeSystem use-after-free problems]

[Jonas Devlieghere via Phabricator via lldb-commits]

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