[PATCH] D40860: [clangd] Fix diagnostic positions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hmm, I looked into the empty diagnostic ranges, and it overlaps here a lot.

Feel free to go ahead and land this and I can merge, otherwise I can include this fix there.
Regardless of how it gets landed, thanks for finding and fixing this, this explains a lot of weird behavior!



================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:124
+/// Convert a clang::SourceLocation to a clangd::Position
+Position SourceLocToClangdPosition(const SourceManager &SrcMgr,
+                                   SourceLocation Location) {
----------------
sammccall wrote:
> nit: functions are `lowerCamelCase`.
> Here and below.
nit: consider just `toPosition`, with the location as the first argument

And similarly below. The shorter names and putting the main param first make the callsites easier to read I think.


================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:200
 public:
-  StoreDiagsConsumer(std::vector<DiagWithFixIts> &Output) : Output(Output) {}
+  StoreDiagsConsumer(const LangOptions &Options,
+                     std::vector<DiagWithFixIts> &Output)
----------------
Actually I think the DiagnosticConsumer interface should pass you the language options itself, if you override the lifecycle methods.


https://reviews.llvm.org/D40860