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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+  ConfigCompile.cpp
   ConfigYAML.cpp
   Diagnostics.cpp
----------------
sammccall wrote:
> 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.
> 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.

if we end up with 6-7 files, I'd prefer to create a subdirectory, which make our source structure a bit tidy, our clangd/ dir already has a lot of source files.



> 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.

sure, I didn't mean we should do this in this patch.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+    if (F.HasUnrecognizedCondition)
+      Conditions.push_back([&](const Params &) { return false; });
+
----------------
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?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:84
+  void compile(Fragment::CompileFlagsBlock &&F) {
+    if (!F.Add.empty()) {
+      std::vector<std::string> Add;
----------------
nit: use early return.


================
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,
----------------
sammccall wrote:
> 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.
Ah, I see. I must have missed that.

btw, could you spell out the auto type for `Error`? I believe it is `DiagKind`, and it would be more clear that it is used in the first parameter of `diag` (given that they are close).


================
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.
----------------
sammccall wrote:
> 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?
not better idea. `config::Env` doesn't seem to be much better... and the signature `apply(const Params &P, Config &C)` seems neat. let's keep it as-is.

The comment of `Path` is a bit vague, it would be nice to clarify what kind of the file we're targeting to? the config file or the .cpp source file?


================
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:
> 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.


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:31
+
+  bool compile() {
+    Conf = Config();
----------------
nit: compileAndApply?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82612