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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:9
+//
+// Various clangd features have configurable behaviour (or can be disabled).
+// The configuration system allows users to control this:
----------------
i think this paragraph belongs to `Config.h`


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:14
+//
+// This file defines the config::Fragment structure which is models one piece of
+// configuration as obtained from a source like a file.
----------------
s/which is models/which models/


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:16
+// configuration as obtained from a source like a file.
+// This is distinct from how the config is interpreted (CompiledFragment),
+// combined (ConfigProvider) and exposed to the rest of clangd (Config).
----------------
again i don't think these are relevant here.

maybe provide more details about any new config option should start propagating from here, as this is the closest layer to user written config,


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:61
+  /// BufferName is used for the SourceMgr and diagnostics.
+  static std::vector<Fragment> parseYAML(llvm::StringRef YAML,
+                                         llvm::StringRef BufferName,
----------------
what about `fromYAML` and returning `vector<const Fragment>` ?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:65
+
+  struct SourceInfo {
+    /// Retains a buffer of the original source this fragment was parsed from.
----------------
why the bundling?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:69
+    /// Shared because multiple fragments are often parsed from one (YAML) file.
+    /// May be null, then all locations are ignored.
+    std::shared_ptr<llvm::SourceMgr> Manager;
----------------
maybe `invalid/absent` rather than `ignored` (or change it to say should be ignored) ? I am assuming this is referring to locations of config parameters like `Add`s and conditions.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:73
+    /// Only valid if SourceManager is set.
+    llvm::SMLoc Location;
+  };
----------------
what is the use of this ?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:75
+  };
+  SourceInfo Source;
+
----------------
why not make this const ? i don't think it makes sense to modify these after creation.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:77
+
+  struct ConditionFragment {
+    std::vector<Located<std::string>> PathMatch;
----------------
comments?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:78
+  struct ConditionFragment {
+    std::vector<Located<std::string>> PathMatch;
+    /// An unrecognized key was found while parsing the condition.
----------------
some comments? especially around `fragment applies to file matching all (or any) of the pathmatches`


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:81
+    /// The condition will evaluate to false.
+    bool UnrecognizedCondition;
+  };
----------------
`HasUnrecognizedCondition` ?

also default init to `true` maybe?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:85
+
+  struct CompileFlagsFragment {
+    std::vector<Located<std::string>> Add;
----------------
some comments.

I am not sure if putting `Fragment` suffix to all of these makes sense, as they already reside inside `Fragment`. Maybe `section` or `block` ? (same goes for ConditionFragment)


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:87
+    std::vector<Located<std::string>> Add;
+  } CompileFlags;
+};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82386