[PATCH] D82386: [clangd] Config: Fragments and parsing from YAML

[Sam McCall via Phabricator via cfe-commits]

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