sammccall marked 9 inline comments as done.
sammccall added a comment.
Thanks for the careful review!
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:73
+ /// Only valid if SourceManager is set.
+ llvm::SMLoc Location;
+ };
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > what is the use of this ?
> > There may be multiple fragments in a file.
> >
> > When tracking how fragments are used in the logs, there should be enough
> > information to identify the particular fragment, without having to guess.
> > - during parsing, we can log in some source-specific way
> > - in the Fragment form, we can log this location to identify the fragment
> > - during compilation, we log this location and the pointer address of the
> > new CompiledFragment
> > - subsequently we can just log the CompiledFragment address
> right, I was thinking that we already have location information available for
> the start of the fragment during parsing and wasn't sure how useful it would
> be in later on stages as we know the exact location of the relevant entries
> (keys, scalars etc.) for logging.
>
> I wasn't against the idea of storing it, just wanted to learn if it has any
> significant importance for the upcoming stages. thanks for the info!
Yeah, storing the start of the fragment given a slightly more correct location
to represent the fragment itself, but more importantly we don't have to hunt
around the config to find something that has a location.
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:77
+
+ struct ConditionFragment {
+ std::vector<Located<std::string>> PathMatch;
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > comments?
> > Whoops, done. There's an argument to be made that we shouldn't maintain
> > docs in two places, and the user docs should be canonical.
> > Something to think about later, maybe we can generate them.
> >
> > I'm always unsure whether to put the doc for "blocks" like this on the
> > struct or the field...
> I suppose it makes sense to put the part starting with ` /// Conditions
> based on a file's path use the following form:` above the field rather than
> the struct, especially if we are planning to generate docs.
>
> also:
> s/ConfigFragment/Fragment/
Yeah - the counter-argument is that if you *are* reading the code then having
the general documentation above the specific stuff reads better. And we can
teach a doc generator/extractor to grab the right text easily enough. I'll
leave it for now, I think.
> s/ConfigFragment/Fragment/
Doh, I did update this everywhere, but apparently not in my brain, so it keeps
coming back...
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:81
+ /// The condition will evaluate to false.
+ bool UnrecognizedCondition;
+ };
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > `HasUnrecognizedCondition` ?
> > >
> > > also default init to `true` maybe?
> > Renamed. I guess you meant `false` here? (an empty condition should not
> > have this set)
> I was actually suggesting `true` to make sure this is explicitly set by the
> parser in case of success and not forgotten when something slips away. but
> not that important.
The problem is the only sensible implementation for a parser is to set it to
false again, then set it to true once you hit an unknown key.
Particularly now that condition is not Optional. I think the invariant that a
default-initialized Fragment corresponds to an empty document is more valuable.
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:87
+ std::vector<Located<std::string>> Add;
+ } CompileFlags;
+};
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > maybe make this an llvm:Optional too. Even though emptiness of `Add`
> > > would be OK in this case, for non-container "blocks" we might need to
> > > introduce an optional, and homogeneity would be nice when it happens.
> > I'd rather push this the other way: the presence of an empty block like
> > this shouldn't have any semantic effect - I think we can be consistent
> > about this and it helps the schema feel regular/extensible/predictable.
> >
> > Condition was the only tricky case, but I don't think we need it, so I've
> > removed the Optional on the condition instead. it's an AND, so an empty set
> > of conditions evaluates to "unconditionally true" anyway.
> sure SG, I was rather afraid of introducing an `int` option one day, then we
> might need to signal it's existence with an `Optional` but one might also
> argue that we would likely have a "default" value for such options, hence
> instead of marking it as `None` we could just make use of the default in
> absence of that option.
Oh definitely I think we'll want `Optional` for singular leaf fields, like
`Optional<bool> BackgroundIndexBlock::Enabled` or whatever.
You want a tristate of yes/no/inherit-existing and Optional is the best fit for
that.
I meant specifically for *these "blocks" that act as namespaces*, we avoid
giving side-effects to the presence/absence, so Optional isn't needed to encode
some user intent.
(Not sure how to formalize this namespace idea, it's something struct-like that
appears as a singular field in something struct-like)?
================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:150
+ for (auto &Child : *S) {
+ if (auto Value = scalarValue(Child, "List item"))
+ Result.push_back(std::move(*Value));
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > shouldn't we error/warn out if not ?
> > we do: scalarValue warns if it returns None and is documented to do so.
> right but this function still returns `Result` and not `None`
Yeah, we ignore the illegal value and continue.
We could make this a hard error, but that is commitment that where we parse a
scalar today we'll never accept anything else in future.
Whereas we might choose to allow some other structure in future (without
picking a new name for the key). LSP is full of this...
On the other hand not seeing a dict where you expect one is a hard error,
because there are fewer reasons to switch from a dict to something else.
================
Comment at: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp:58
+ Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+ if (!D.getRanges().empty()) {
+ const auto &R = D.getRanges().front();
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > is there an explanation to what these ranges are ?
> > >
> > > All I could find is somewhere in the implementation making sure these are
> > > relative to the `D.getLineNo` and nothing else :/
> > These are ranges to highlight (e.g. in console output). SourceMgr/SMDiag is
> > modeled after clang's SourceManager/Diagnostic, where this stuff is a bit
> > better documented.
> right, sorry i forgot to mention in the first comment. I was rather afraid of
> Range being relative to start of line, but being contained in another line.
> (due to the usage on start.character and end.character down below)
Oh yeah that threw me for a loop too. I think I got the semantics here right,
in any case it's only tests (we don't own any of the code *generating* the
ranges, and we're not consuming it in production code yet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82386/new/
https://reviews.llvm.org/D82386
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
