sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+ if (F.HasUnrecognizedCondition)
+ Conditions.push_back([&](const Params &) { return false; });
+
----------------
hokein wrote:
> 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.
> so the member Condition in Fragment will be enhanced to something like
> std::vector<XXX> Conditions in the future?
Ah, not quite...
Fragment is supposed to closely follow the YAML format, which is:
`If: { PathMatch: foo, Platform: bar }`
not
`If: [ {PathMatch: foo}, {Platform: bar} ]`
That is, the partitioning of parts of the `If` block into `Conditions` is
implicit rather than explicit. (It's basically partitioning on the map keys,
but maybe there'll be exceptions).
The subtle difference between Fragment::Condition and
CompiledFragmentImpl::Condition is confusing though. I think renaming the
former to Fragment::If is much clearer (not sure why I didn't name it that in
the first place).
> Since Fragment and CompiledFragment is 1:1 mapping, I think it is important
> to keep their implementation aligned
I think the fragment and the fragmentcompiler are pretty well aligned, but I
don't think duplicating the fragment structure in the final
CompiledFragmentImpl is a good idea.
The reason is captured pretty well in the essay "Parse, don't validate".
Through compilation here we want to examine the config and establish that we
can apply this configuration efficiently, and without any more errors. The
type-driven way to do that is to emit a function that applies the configuration
and whose signature doesn't allow for errors.
https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:46
+ if (!C(P)) {
+ dlog("config::Fragment {0}: condition not met", this);
+ return false;
----------------
hokein wrote:
> 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.
I've changed these from config::Fragment to "Config fragment" to be less
misleading. I'm not sure sprinkling in internal class names helps much vs
having them be clear sentences, I always end up searching for the log string to
see exactly where it comes from anyway.
This is dlog, it would be nice to make it vlog but I think it's just way too
spammy, we'll get several of these for every file that's opened and
background-indexed...
> we might need a tracing mechanism (at least some useful logs)
My secret plan is to have a clangd flag like `clangd --check <filename>` that
will *not* run the language server, but instead dump a bunch of information
about how clangd would process that file (stringing together CDB, config,
query-driver, diagnostics...). This would miss config-over-LSP but in every
other respect seems much simpler than trying to find ways to extract this stuff
live and get it in front of the user somehow.
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:35
+#include "ConfigProvider.h"
#include "llvm/ADT/Optional.h"
----------------
hokein wrote:
> the dependence ( `ConfigFragment.h` depends on `ConfigProvider.h`) seems a
> bit counterintuitive...
Agree, it seems backwards - this is the one thing that seemed cleaner about the
old factoring.
But it does appear that everything is in the right file, the dependency is
needed, and a (circular) dependency in the other direction won't be needed
so... maybe we can live with it?
The way I think of it is that ConfigProvider is defining an entirely abstract
interface that ConfigFragment is implementing.
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
