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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+    if (F.HasUnrecognizedCondition)
+      Conditions.push_back([&](const Params &) { return false; });
+
----------------
sammccall wrote:
> hokein wrote:
> > I think if this case happened, we might just bail out directly -- adding the regex conditions seems unnecessary, we never execute them on Line 127 (but we might still need computing the regex for diagnostics).
> > 
> > and I'm not sure the motivation of using `vector` for `Conditions` and `Apply`, it looks like Apply is just 1 element (if we do the bailout for conditions, `Conditions` will be at most 1 element).
> > 
> > I feel like using a single unique_function for `Conditions` and `Apply` might be a better fit with `Fragment`. There is a comment in `ConfigFragment.h`:
> > 
> > >  Each separate condition must match (combined with AND).
> > >  When one condition has multiple values, any may match (combined with OR).
> >  
> > so my reading is that:
> > 
> > - `Fragment` and `CompiledFragment` is a 1:1 mapping
> > - A `Fragment::Condition` represents a single condition, so the AND match applies for different `Fragment`s, which is in ConfigProvider?
> > - A single condition can have multiple values (`PathMatch`), we use OR match
> > 
> > Is that correct?
> > I think if this case happened, we might just bail out directly -- adding the regex conditions seems unnecessary, we never execute them on Line 127 (but we might still need computing the regex for diagnostics).
> >
> > and I'm not sure the motivation of using vector for Conditions and Apply, it looks like Apply is just 1 element (if we do the bailout for conditions, Conditions will be at most 1 element).
> 
> So the explanation for both of these things is that this file is going to grow a lot, and in particular these sections handling PathMatch etc are going to occur again and again, so we need a scalable pattern. I've deliberately limited the scope of the first patchset to only support one condition, one setting to apply etc, but you have to imagine there are likely to be 5 conditions and maybe 25 settings.
> 
> So with that in mind:
>  - yes, actually saving the conditions isn't necessary, but we do need to evaluate them for side-effects (diagnostics) and it's simpler not to keep track of them. Optimizing performance of broken configs is not a big win :-)
>  - it's marginally simpler to have only one Apply function, but much simpler if separate parts of compile() can push settings independently. Same goes for condition in fact...
> 
> > Fragment and CompiledFragment is a 1:1 mapping
> 
> Yes. I think this is somewhere where the naming is helpful at least :-)
> 
> > A Fragment::Condition represents a single condition, so the AND match applies for different Fragments, which is in ConfigProvider?
> 
> The AND match applies to the multiple Conditions in a single CompiledFragment. Each fragment only considers its own conditions. 
> So given:
> ```
> If:
>   Platform: win
>   PathMatch: [foo1/.*, foo2/.*]
> CompileFlags:
>   Add: -foo
> ---
> CompileFlags:
>   Add: -bar
> ```
> 
> We end up with... one Provider, suppling two CompiledFragments (each compiled from a separate Fragment).
> 
> The first CompiledFragment has two Conditions, conceptually:
>  - `Params.Platform == "win"`
>  - `Regex("foo1/.*").matches(Params.File) || Regex("foo2/.*").matches(Params.File)`
> 
> The second CompiledFragment has no Conditions. Each fragment has one Apply function.
> 
> The CompiledFragments are applied independently. They each consider their own Conditions, ANDing them together.
> 
> So CompiledFragment #1 checks both conditions. If any fails, it bails out. Otherwise it runs its Apply function, adding `-foo`.
> 
> Then CompiledFragment #2 runs. It has no conditions, so never bails out. It adds `-bar`.
oh, thanks for the explanation, this helps me a lot to understand the code.

so the member `Condition` in `Fragment` will be enhanced to something like `std::vector<XXX> Conditions` in the future? If so, can we defer that for `CompiledFragment` as well? 


Since `Fragment` and `CompiledFragment` is 1:1 mapping, I think it is important to keep their implementation aligned, it'd help a lot for readers to understand the code. The condition logic (AND, OR) is subtle and complicated,  I was reading this part of code back and forth, still inferred it in an incorrect way.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:40
+
+struct Impl {
+  std::vector<llvm::unique_function<bool(const Params &) const>> Conditions;
----------------
nit: I'd use a more verbose name, `CompiledFragmentImpl`.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:46
+      if (!C(P)) {
+        dlog("config::Fragment {0}: condition not met", this);
+        return false;
----------------
maybe `Impl::applying config ...` to allow better debug logging?

this reminds us: since the config can easily become a monster, think about multiple config files from different sources (user config file, project config file, LSP, flags) are applied together, we might need a tracing mechanism (at least some useful logs) to help us diagnose a final complicated config. no need to do anything in this patch, just something we should keep in mind.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:126
+CompiledFragment Fragment::compile(DiagnosticCallback D) && {
+  llvm::StringRef SourceFile = "<unknown>";
+  std::pair<unsigned, unsigned> LineCol = {0, 0};
----------------
nit: maybe name it ConfigFile, SourceFile sounds like a .cpp file.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:35
 
+#include "ConfigProvider.h"
 #include "llvm/ADT/Optional.h"
----------------
the dependence ( `ConfigFragment.h` depends on `ConfigProvider.h`) seems a bit counterintuitive...


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:81
+  /// The returned function is a cheap-copyable wrapper of refcounted internals.
+  CompiledFragment compile(DiagnosticCallback) &&;
+
----------------
this API is neat, thanks!


================
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:
> 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.
SG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82612