[PATCH] D44764: [clangd] Use a header for common inclusions for tests

Wed Mar 28 19:45:07 PDT 2018

malaperle added a comment.

In https://reviews.llvm.org/D44764#1046766, @sammccall wrote:

> I understand the template instantiations here are a mess and we need to solve the problem. However I'm not sure this is the right direction:
>
> - it violates IWYU, and we expect people consistently to get gmock transitively via this header. This goes against common practice, is inconsistent with the rest of LLVM, and confuses tools (including clangd itself!). It would need to be carefully documented, but I don't think this is enough.

I didn't know LLVM code was IWYU, is this mentioned in the guide-line? I would think for tests this can be more lax maybe. I'm curious what you mean though by confusing the tools?

>   - it's fragile - as soon as one test includes gmock directly we have this problem again
>   - `printers.h` takes a horizontal slice of the types we have - it seems to suffer from the same problem as "not all tests should depend on clangdserver".
> 
>     I'd suggest instead:
> - where there's a fairly nice general-purpose debugging representation of a type, we should name that `operator<<` and declare it in the header alongside the type. That way it's clear that it's available, there's no risk of ODR violation, and it can be used for debugging as well as by gtest.
>   - where there isn't one (or it's insufficiently detailed), we should use something more primitive like `EXPECT_THAT(foo, ...) << dump(foo)` with `dump` defined locally in the cpp file. We can't share these, but these aren't really sharable anyway. This isn't ideal (nested matcher messages) but should be good enough in practice.

Yes, it is fragile but even using operator<<, we can still have the problem of anyone adding a Printer, which is normally how gtest is used. So I think either way is going to be fragile. But I don't mind either way, so I'll change the patch for operator<<

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44764