[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:47
+  /// This always produces a usable compiled fragment (errors are recovered).
+  explicit CompiledFragment(Fragment, DiagnosticCallback);
+
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > Would it be more nature to have a function `compile`(or so) to do the actual compile (Fragment -> CompiledFragment) rather than doing it in a constructor?
> > > I'm not sure. The idea that the input/result is Fragment/CompiledFragment, and that the compilation cannot fail, suggests to me that a constructor is OK here.
> > > On the other hand, the DiangosticCallback param (which we don't keep a reference to) is a bit weird.
> > > 
> > > So I don't really feel strongly about it one way or the other, happy to change it if you do think it would be significantly better.
> > You point is fair. 
> > I'm not sure using `compile` is *significantly* better, but it is better IMO -- conceptually, the class name `CompiledFragment` implies that there should be a `compile` API (this was my impression when reading the code at the first time)
> > 
> > it might be ok to keep as-is.
> Argh, you convinced me this would be better, but I can't come up with a form I like.
> 
> - `static CompiledFragment::compile` repeats itself in a weird way. But any other option requires messing around with friend functions, which feels off
> - `Fragment::compile` is maybe the most natural, but the dependencies are backwards (needs the CompiledFragment type, it gets implemented in the wrong file...).
> - standalone `compile()` is maybe the most palatable, but seems a little undiscoverable
> - having a function return CompiledFragment means that putting it into a shared_ptr is annoying work, and breaks our pointer-logging trick, but having the factory function return a pointer also seems odd.
> 
> I'm starting to think maybe the CompiledFragment class shouldn't be public at all, and we should just `using CompiledFragment = unique_function<bool(const Params&, Config&) const>`. WDYT?
> I'm starting to think maybe the CompiledFragment class shouldn't be public at all

Thinking more about this:

 - making it std::function<...> instead lets us wrap the shared_ptr inside which makes a few APIs a bit cleaner
 - as its an interface type, this leaves config providers free to use other ways of manipulating config rather than going via fragments, which is potentially nice for embedders
 - the main downside is we're committing to not exposing any more structure (e.g. querying a fragment for which directory it applies to)

starting to quite like this idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82612