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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
