[PATCH] D63222: [Clangd] Fixed clangd diagnostics priority

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Mostly LG, just a few re-orderings to make code more readable and get rid of redundant Lexer calls.



================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:108
+  // Otherwise use zero width insertion range
   auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
+  if (R.isValid()) {
----------------
since we are going to lex again in case of `R.isValid()`, you can get rid of this lexer call.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:109
   auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
-  if (!R.isValid()) // Fall back to location only, let the editor deal with it.
+  if (R.isValid()) {
+    // check if token at location is a priority i.e. not a comment
----------------
let's change this condition to rather check for `Lexer::getRawToken` succeeded and underlying token `isNot(tok::comment)`.
Then you can simply construct the range with `CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc())`.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:111
+    // check if token at location is a priority i.e. not a comment
+    Token token;
+    bool isTokenPriority = false;
----------------
LLVM style uses `UpperCammelCase` for variable names. so this should be something like, `Token Tok;`, same for the bool below.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:119
+  else if (FallbackRange)
+    return *FallbackRange;
+  else // Fall back to location only, let the editor deal with it.
----------------
the FallbackRange(if set) and `CharSourceRange::getCharRange(Loc);` are the same.

So you can get rid of `FallbackRange` and merge the last two branches of this conditional.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63222