[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/ConfigYAML.cpp:29
+  llvm::SourceMgr &SM;
+  llvm::SmallString<256> Buf;
+
----------------
a comment for this buf, especially the reason why it's a member rather than a function-local when needed. (I suppose to get rid of const/dest penalties?)


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:40
+    Dict.handle("If", [&](Node &N) {
+      F.Condition.emplace();
+      return parse(*F.Condition, N);
----------------
It is a little bit subtle that "at most once" execution of this handler is assured by DictParser. Can we put some comments explaining it to either DictParser itself or to `handle` member ?


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:74
+    llvm::StringRef Description;
+    std::vector<std::pair<llvm::StringRef, std::function<bool(Node &)>>> Keys;
+    std::function<void(llvm::StringRef)> Unknown;
----------------
maybe a comment for these two, suggesting the latter is used for unknown keys.

and also mention it is handlers responsibility emit any diagnostics in case of failures?


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:84
+    void handle(llvm::StringLiteral Key, std::function<bool(Node &)> Parse) {
+      Keys.emplace_back(Key, std::move(Parse));
+    }
----------------
maybe assert on duplicate keys in debug builds?


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:100
+        if (!K)
+          return false;
+        auto Key = Outer->scalarValue(*K, "Dictionary key");
----------------
i suppose yamlparser has already emitted a diagnostic by then ? can you add a comment about it if that's the case.

and if not, shouldn't we emit something about a broken stream?

same goes for `KV.getValue()` below.


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:105
+        if (!Seen.insert(**Key).second) {
+          Outer->warning("Duplicate key " + **Key, *K);
+          continue;
----------------
maybe `Ignoring duplicate key` instead of just `Duplicate key` ?


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:136
+    else if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
+      return Located<std::string>(S->getValue(Buf).str(), N.getSourceRange());
+    warning(Desc + " should be scalar", N);
----------------
s/S->getValue(Buf)/BS->getValue()/  (or change `auto *BS` to `auto *S`)

and a test


================
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));
----------------
shouldn't we error/warn out if not ?


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:161
+  // Report a "hard" error, reflecting a config file that can never be valid.
+  bool error(const llvm::Twine &Msg, const Node &N) {
+    SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Error, Msg,
----------------
do we really want to return a boolean here (and not at the warning ?)

I can see that it's nice to `return error("xxx")` but it looks surprising as we are not returning the error but just false.
I would make this void, up to you though.


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:202
+  // SM has two entries: "main" non-owning buffer, and ignored owning buffer.
+  SM->AddNewSourceBuffer(std::move(Buf), llvm::SMLoc());
+  return Result;
----------------
*sigh*

what a mess, Scanner in YamlParser already adds this buffer. It is unclear why though, as it adds a non-owning buffer and that buffer is never accessed.  (well apart from SMLoc to Buffer conversions when emitting diags)
It rather makes use of pointers to the underlying data, obtained before adding that buffer. So it is the caller keeping the data alive anyways.

should we consider having an entry point that takes an SMLoc and a SourceMgr for `yaml::Stream` instead of this? That way we can add the owning entry to sourcemanager and start the stream at beginning of that buffer.

i am afraid this might bite us in the future, as SM is exposed publicly and one might try to iterate over buffers, which might come as a shock. we can postpone until that happens though, so feel free to leave it as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82386