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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks! Looks great, I've finally managed to finish all my iterations :D

Apart from the comments(IIRC which are mostly minor), it felt like there were a few lines that aren't clang-format'd. Please make sure to run clang-format on the files at some point.



================
Comment at: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp:58
+      Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+      if (!D.getRanges().empty()) {
+        const auto &R = D.getRanges().front();
----------------
is there an explanation to what these ranges are ?

All I could find is somewhere in the implementation making sure these are relative to the `D.getLineNo` and nothing else :/


================
Comment at: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp:73
+
+    friend void PrintTo(const Diag &D, std::ostream *OS) { *OS << D.Message; }
+  };
----------------
why no print others ?

`Kind: D.Message @ Pos - Range`


================
Comment at: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp:133
+  ASSERT_THAT(Diags.Diagnostics,
+              ElementsAre(DiagMessage("Unknown Condition key UnknownCondition"),
+                          DiagMessage("Unexpected token. Expected Key, Flow "
----------------
i think having a full matcher(i.e with kind, pos and ranges) might be more expressive here, up to you though


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82386