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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
