[Lldb-commits] [PATCH] D70775: [lldb] Add MockTypeSystem and some basic unit test for CompilerType

Wed Nov 27 08:36:15 PST 2019

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D70775#1761597 <https://reviews.llvm.org/D70775#1761597>, @teemperor wrote:

> I tried moving this to gmock, but after an hour of not being able to even declare a simple virtual method due to issues like this <https://github.com/google/googletest/issues/533> I don't think this approach is really practical. gmock is a nightmare to debug with all these completely undocumented macros. If gmock was actually documented and would work, then I think we should use it. But at the current state I can't even get it to compile code it seems without having to diagnostic-disable hacks, so that's a no-go

That's easy to work around by just not using any "override" keywords in the class which defines the mock methods. This is unfortunate, but I don't think it's a showstopper. If anything, it's a reason for updating to a newer gmock version, since that's solved there.

I'm not sure what documentation you were looking at, but overall, I've found googlemock to be documented <https://chromium.googlesource.com/external/github.com/google/googletest/+/HEAD/googlemock> pretty well (definitely better than llvm or lldb).

So, I don't think either of these things are a reason to *not use* it. However, they are not reasons for *using* it either. Gmock is good for some cases, and not so good for others. Right now it's not clear to me what kind of uses you have in mind, but in the simple tests you have in this patch, it is true that gmock would not provide any value. We can revisit that decision later, or add a new gmock-based type system class (there's nothing stopping us from having more than one).

> 
> 
>   Also we anyway also want to model things like fake type hierarchies that one can traverse via CompilerType/TypeSystem (e.g. when doing completions like in the Variable completion code), so simple mocking is anyway not really the right fit.

Yes, that is something where gmock would not be particularly useful..

> Changes:
> 
> - Renamed MockTypeSystem -> FakeTypeSystem to reflect that this is not just mocking.
> - Moved TestingSupport library to its own subdirectory and library that links against Symbol.

Thanks.

> - Replaced `llvm_unreachable` in the unimplemented functions with macro to make migration easier if someone has a better approach to handling them.

I don't think this brings any value (the "verboseness" of `llvm_unreachable("unimplemented")` was not my main issue with this approach). Let's just revert that.

================
Comment at: lldb/unittests/TestingSupport/Symbol/FakeTypeSystem.h:22
+
+#define UNIMPLEMENTED_METHOD llvm_unreachable("not implemented")
+
----------------
I don't think this is worth a macro. If you really want to save a couple of characters you can just declare a `unimplemented` static member function...

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

https://reviews.llvm.org/D70775