[PATCH] D54475: [clangd] Allow observation of changes to global CDBs.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: clangd/Function.h:147
+private:
+  static_assert(std::is_same<typename std::decay<T>::type, T>::value,
+                "use a plain type: event values are always passed by const&");
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > NIT: Maybe move this static_assert to the top of the class?
> > I'd argue this is part of the public interface as it puts constraints on the template parameters.
> On balance I don't think I agree here.
> 
> I'm not sure about having the static assert at all, unless we have reason to believe it will compile but do the wrong thing. Happy to compromise by including it, but I don't think it belongs up front.
> It's not very human-readable, and computers are just as happy with it in the private section.  It's consistent with some other ways of disallowing compilation, like declaring members private.
Reference collapsing rules can bite us here, e.g. I don't think making `Event<int&>` and `Event<int&&>` valid makes sense, even though both would compile.

> It's consistent with some other ways of disallowing compilation, like declaring members private.
Would also argue those private members should be declared at the top of the class for the same reasons.

But we just have different opinions here, it's not terribly important.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54475