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