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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 10 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+  ConfigCompile.cpp
   ConfigYAML.cpp
   Diagnostics.cpp
----------------
hokein wrote:
> we have a few config-related files now, I wonder would it make sense to create a config/ subdir?
It seems marginal to me - could go either way. We have a small handful (2 headers and 3 impl files). There may be 1-2 more, but I'm not sure yet. (config-files-on-disk, and json->fragment).
So 6-7 files... smaller than the other subdirs, but not tiny.

There's also the fact that config.h/cpp is not like the others: all features (not just infra) depend on this, it's not in the `config::` namespace etc. So should it really be in the subdir?

Mind if we hold off on this until at least the scope of D82335 has landed? I think it would be awkward to do this move halfway through this patchset, but fairly easy to do at any point that things are stable.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:96
+
+  constexpr static auto Error = llvm::SourceMgr::DK_Error;
+  void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
----------------
hokein wrote:
> this member seems unused.
it's used on line 52 `diag(Error, ...)`.

I wouldn't bother extracting it except there should be a bunch more places where errors are emitted, and keeping them readable is worthwhile IME.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:32
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+  /// Absolute path to file we're targeting. Unix slashes.
----------------
hokein wrote:
> nit: I find the name `Params` is too general, it implies less information. When I read the code using `Params`, I have to go-to-definition to see what it means. 
> 
> if we only have a single member in this struct, maybe just use it directly.
Agree with the vague name.
In most contexts, the name is `config::Params` which is at least a bit more specific.

> if we only have a single member in this struct, maybe just use it directly.

I don't like this, though - there are likely to be more things here in the future (e.g. platform), and passing around a string is much less clear as to intent. So any idea about a name for this struct?

What about `config::Environment` or `config::Env`? still too vague?


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:40
+///
+/// Fragments are compiled by Providers when first loaded, and cached for reuse.
+/// Like a compiled program, this is good for performance and also encourages
----------------
hokein wrote:
> I might miss the context, where is the provider, not added yet? 
Right, D82335 has a rought draft of everything together (it's called ConfigProvider in that patch).

I think it's less churn-y to add the documentation as the classes are added, even if it means temporarily referring to something that doesn't exist yet.


================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:47
+  /// This always produces a usable compiled fragment (errors are recovered).
+  explicit CompiledFragment(Fragment, DiagnosticCallback);
+
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:40
+TEST_F(ConfigCompileTests, Condition) {
+  // No condition.
+  Frag.CompileFlags.Add.emplace_back("X");
----------------
hokein wrote:
> my first impression was that each `// XXX` is a separated test, but it turns out not, the tests here seem to be stateful -- `Frag` keeps being modified. I think it is intentional, but it is hard for readers to track given that the code is long.
Agree. Made this test reset fragment state after each group of assertions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82612