[Lldb-commits] [PATCH] D76011: Add a verification mechanism to CompilerType.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I think this change is slightly orthogonal to the way we are supposed to construct types in LLDB. After the quirky bugs we had with Decl * and DeclContext * pointers in the CompilerDeclContext, I moved all the `Compiler*(TypeSystem *, opaque_ptr*)` constructors calls into wrapper functions in TypeSystemClang where we could use the real types. This avoids all the pitfalls from having to pass void pointers to the constructor which breaks when multiple inheritance and implicit type conversion is involved. I also added a remark to the documentation of all these constructors to not use them directly but use the wrapper functions in the respective TypeSystem.

Because of this, this change is mostly a no-op upstream as TypeSystemClang already does this check for CompilerTypes (and also CompilerDeclContexts) in these wrapper functions. Now we essentially check every CompilerType twice (once in the wrapper function which only exists for Clang now and which is typesafe. And now also in the non-typesafe CompilerType constructor which is called by the wrapper function). My plan was to actually completely hide this constructor and only make it accessible to TypeSystem subclasses via some wrapper function, so in theory this check could safely happen in a less intrusive way in the TypeSystem* wrapper function. See for example `TypeSystemClang::CreateDeclContext`.

On another note, the verify function should in my opinion return a `void` and just assert directly. The main reason being that currently we only get an assertion that `Verify()` failed but this doesn't tell me what specific check has failed in the backend. I don't see any code that could check the bool result of `Verify` and do anything useful with it (beside asserting).

Also I'm very confused why a nullptr type with a valid type system is not triggering asserts? Do we actually have these types anywhere (and if yes, that would be kinda fishy IMHO).

Beside those points I think this change is an improvement over the current situation so I'm fine with keeping it. But in general I would much rather see Swift use a type-safe wrapper function instead and only keep this patch around until this has happened everywhere.

(Sorry for the late review, still working through my email backlog)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76011/new/

https://reviews.llvm.org/D76011