[NFC][clangd] cleanup llvm-else-after-return findings

[kuhnel]

Summary

Cleanup of clang-tidy findings: removing "else" after a return statement
to improve readability of the code.

This patch was created by applying the clang-tidy fixes automatically.

[sammccall]

Mostly looks good. There's a bunch of ifs that are now trivial (condition and body each fit on one line, no else) and we'd usually drop braces there, but it's probably not worth the hassle.

I have to say that this rule slightly improves "early exit after error condition" type code, but makes it less clear that "there are 3 cases" type code is exhaustive.
So personally I'm not sure the check is a win if we want to follow it everywhere, I'd consider fixing the cases where it improves things and then turning it off. @kadircet am I being too picky here?

clang-tools-extra/clangd/JSONTransport.cpp

260

This seems inconsistent: we're elseing after continue.

If we've using this style, i'd invert the if here so the "continue" case just falls off the end of the loop body. (But keep the comment)

283

Looks like there are unrelated formatting changes in a couple of files

clang-tools-extra/clangd/Selection.cpp

350

FWIW I find this one *much* less clear :-(
I'd personally prefer to ignore the rule in this case, but curious what others think

[sammccall]

Discussed offline, I believe the plan is to NOLINT the locations where we're handling several cases exhaustively.

clang-tools-extra/clangd/Selection.cpp

350

I think this is a nolint

[kuhnel]

addressed Sam's comments

Sorry for the continued mess with automatic code formatting. Somehow VSCode forgot my settings :(

[sammccall]

Thanks!

clang-tools-extra/clangd/ClangdLSPServer.cpp

Server->documentSymbols(
       Params.textDocument.uri.file(),
       [this, FileURI, Reply = std::move(Reply)](
           llvm::Expected<std::vector<DocumentSymbol>> Items) mutable {
         if (!Items)
           return Reply(Items.takeError());
         adjustSymbolKinds(*Items, SupportedSymbolKinds);
         if (SupportsHierarchicalDocumentSymbol)
           return Reply(std::move(*Items));
-        else
-          return Reply(flattenSymbolHierarchy(*Items, FileURI));
       });
 }
 
 void ClangdLSPServer::onFoldingRange(
     const FoldingRangeParams &Params,
     Callback<std::vector<FoldingRange>> Reply) {
   Server->foldingRanges(Params.textDocument.uri.file(), std::move(Reply));
 }

clang-tools-extra/clangd/FuzzyMatch.cpp

llvm::SmallString<256> FuzzyMatcher::dumpLast(llvm::raw_ostream &OS) const {
   }
   if (WordN == 0) {
     OS << "Word is empty: no match.\n";
     return Result;
   }
   if (!WordContainsPattern) {
     OS << "Substring check failed.\n";
     return Result;
-  } else if (isAwful(std::max(Scores[PatN][WordN][Match].Score,
-                              Scores[PatN][WordN][Miss].Score))) {
+  }
+  if (isAwful(std::max(Scores[PatN][WordN][Match].Score,
     OS << "Substring check passed, but all matches are forbidden\n";
   }
   if (!(PatTypeSet & 1 << Upper))
     OS << "Lowercase query, so scoring ignores case\n";
 
   // Traverse Matched table backwards to reconstruct the Pattern/Word mapping.
   // The Score table has cumulative scores, subtracting along this path gives
   // us the per-letter scores.

clang-tools-extra/clangd/Hover.cpp

std::string getLocalScope(const Decl *D) {
   std::vector<std::string> Scopes;
   const DeclContext *DC = D->getDeclContext();
 
   // ObjC scopes won't have multiple components for us to join, instead:
   // - Methods: "-[Class methodParam1:methodParam2]"
   // - Classes, categories, and protocols: "MyClass(Category)"
   if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(DC))
     return printObjCMethod(*MD);
-  else if (const ObjCContainerDecl *CD = dyn_cast<ObjCContainerDecl>(DC))
+  if (const ObjCContainerDecl *CD = dyn_cast<ObjCContainerDecl>(DC))
     return printObjCContainer(*CD);
 
   auto GetName = [](const TypeDecl *D) {
     if (!D->getDeclName().isEmpty()) {
       PrintingPolicy Policy = D->getASTContext().getPrintingPolicy();
       Policy.SuppressScope = true;
       return declaredType(D).getAsString(Policy);
     }

clang-tools-extra/clangd/JSONTransport.cpp

bool JSONTransport::handleMessage(llvm::json::Value Message,
   }
   // Params should be given, use null if not.
   llvm::json::Value Params = nullptr;
   if (auto *P = Object->get("params"))
     Params = std::move(*P);
 
   if (ID)
     return Handler.onCall(*Method, std::move(Params), std::move(*ID));
-  else
-    return Handler.onNotify(*Method, std::move(Params));
 }
 
 // Tries to read a line up to and including \n.
 // If failing, feof(), ferror(), or shutdownRequested() will be set.
 bool readLine(std::FILE *In, llvm::SmallVectorImpl<char> &Out) {
   // Big enough to hold any reasonable header line. May not fit content lines
   // in delimited mode, but performance doesn't matter for that mode.
   static constexpr int BufSize = 128;
while (true) {
     if (LineRef.consume_front("Content-Length: ")) {
       if (ContentLength != 0) {
         elog("Warning: Duplicate Content-Length header received. "
              "The previous value for this message ({0}) was ignored.",
              ContentLength);
       }
       llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
       continue;
-    } else if (!LineRef.trim().empty()) {
+    }
+    if (!LineRef.trim().empty()) {
       // It's another header, ignore it.
       continue;
     } else {
       // An empty line indicates the end of headers.
       // Go ahead and read the JSON.
       break;
     }
   }
 
   // The fuzzer likes crashing us by sending "Content-Length: 9999999999999999"
   if (ContentLength > 1 << 30) { // 1024M
     elog("Refusing to read message with long Content-Length: {0}. "
          "Expect protocol errors",
          ContentLength);
     return false;
   }
   if (ContentLength == 0) {
     log("Warning: Missing Content-Length header, or zero-length message.");
     return false;
   }
 
   JSON.resize(ContentLength);
   for (size_t Pos = 0, Read; Pos < ContentLength; Pos += Read) {
     // Handle EINTR which is sent when a debugger attaches on some platforms.
-    Read = retryAfterSignalUnlessShutdown(0, [&]{
-      return std::fread(&JSON[Pos], 1, ContentLength - Pos, In);
-    });
+    Read = retryAfterSignalUnlessShutdown(
+        0, [&] { return std::fread(&JSON[Pos], 1, ContentLength - Pos, In); });
     if (Read == 0) {
       elog("Input was aborted. Read only {0} bytes of expected {1}.", Pos,
            ContentLength);
       return false;
     }
     InMirror << llvm::StringRef(&JSON[Pos], Read);
     clearerr(In); // If we're done, the error was transient. If we're not done,
                   // either it was transient or we'll see it again on retry.

clang-tools-extra/clangd/PathMapping.cpp

const std::string &From = Dir == PathMapping::Direction::ClientToServer
                                   ? Mapping.ClientPath
                                   : Mapping.ServerPath;
     const std::string &To = Dir == PathMapping::Direction::ClientToServer
                                 ? Mapping.ServerPath
                                 : Mapping.ClientPath;
     llvm::StringRef Body = Uri->body();
     if (Body.consume_front(From) && (Body.empty() || Body.front() == '/')) {
       std::string MappedBody = (To + Body).str();
-      return URI(Uri->scheme(), Uri->authority(), MappedBody)
-          .toString();
+      return URI(Uri->scheme(), Uri->authority(), MappedBody).toString();
     }
   }
   return llvm::None;
 }
 
 void applyPathMappings(llvm::json::Value &V, PathMapping::Direction Dir,
                        const PathMappings &Mappings) {
   using Kind = llvm::json::Value::Kind;

 };
 
 // Converts a unix/windows path to the path portion of a file URI
 // e.g. "C:\foo" -> "/C:/foo"
 llvm::Expected<std::string> parsePath(llvm::StringRef Path) {
   namespace path = llvm::sys::path;
   if (path::is_absolute(Path, path::Style::posix)) {
     return std::string(Path);
-  } else if (path::is_absolute(Path, path::Style::windows)) {
+  }
+  if (path::is_absolute(Path, path::Style::windows)) {
     std::string Converted = path::convert_to_slash(Path, path::Style::windows);
     if (Converted.front() != '/')
       Converted = "/" + Converted;
     return Converted;
   }
   return error("Path not absolute: {0}", Path);
 }
 

clang-tools-extra/clangd/Protocol.cpp

bool fromJSON(const llvm::json::Value &Params, ExecuteCommandParams &R,
   const auto *ArgsArray = Args->getAsArray();
   if (!ArgsArray) {
     P.field("arguments").report("expected array");
     return false;
   }
   if (ArgsArray->size() > 1) {
     P.field("arguments").report("Command should have 0 or 1 argument");
     return false;
-  } else if (ArgsArray->size() == 1) {
+  }
+  if (ArgsArray->size() == 1) {
     R.argument = ArgsArray->front();
   }
   return true;
 }
 
 llvm::json::Value toJSON(const SymbolInformation &P) {
   llvm::json::Object O{
       {"name", P.name},

clang-tools-extra/clangd/Selection.cpp

testChunk(FileID FID, llvm::ArrayRef<syntax::Token> Batch) const {
     // Handle tokens that were passed as a macro argument.
     SourceLocation ArgStart = SM.getTopMacroCallerLoc(StartLoc);
     if (SM.getFileID(ArgStart) == SelFile) {
       if (isFirstExpansion(FID, ArgStart, SM)) {
         SourceLocation ArgEnd =
             SM.getTopMacroCallerLoc(Batch.back().location());
         return testTokenRange(SM.getFileOffset(ArgStart),
                               SM.getFileOffset(ArgEnd));
-      } else {
-        /* fall through and treat as part of the macro body */
       }
+      /* fall through and treat as part of the macro body */
     }
 
     // Handle tokens produced by non-argument macro expansion.
     // Check if the macro name is selected, don't claim it exclusively.
     auto Expansion = SM.getDecomposedExpansionLoc(StartLoc);
     if (Expansion.first == SelFile)
       // FIXME: also check ( and ) for function-like macros?
       return testToken(Expansion.second);
-    else
-      return NoTokens;
   }
 
   // Is the closed token range [Begin, End] selected?
   SelectionTree::Selection testTokenRange(unsigned Begin, unsigned End) const {
     assert(Begin <= End);
     // Outside the selection entirely?
     if (End < SpelledTokens.front().Offset ||
         Begin > SpelledTokens.back().Offset)

clang-tools-extra/clangd/TUScheduler.cpp

while (true) {
       assert(!CurrentRequest && "A task is already running, multiple workers?");
       for (auto Wait = scheduleLocked(); !Wait.expired();
            Wait = scheduleLocked()) {
         assert(PreambleRequests.empty() &&
                "Preamble updates should be scheduled immediately");
         if (Done) {
           if (Requests.empty())
             return;
-          else     // Even though Done is set, finish pending requests.
-            break; // However, skip delays to shutdown fast.
+          // Even though Done is set, finish pending requests.
         }
 
         // Tracing: we have a next request, attribute this sleep to it.
         llvm::Optional<WithContext> Ctx;
         llvm::Optional<trace::Span> Tracer;
         if (!Requests.empty()) {
           Ctx.emplace(Requests.front().Ctx.clone());
           Tracer.emplace("Debounce");

clang-tools-extra/clangd/XRefs.cpp

if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
       if (CMD->isPure()) {
         if (TouchedIdentifier && SM.getSpellingLoc(CMD->getLocation()) ==
                                      TouchedIdentifier->location()) {
           VirtualMethods.insert(getSymbolID(CMD));
           LocateASTReferentMetric.record(1, "method-to-override");
         }
       }
       // Special case: void foo() ^override: jump to the overridden method.
       if (NodeKind.isSame(ASTNodeKind::getFromNodeKind<OverrideAttr>()) ||
           NodeKind.isSame(ASTNodeKind::getFromNodeKind<FinalAttr>())) {
         // We may be overridding multiple methods - offer them all.
         for (const NamedDecl *ND : CMD->overridden_methods())
           AddResultDecl(ND);
         continue;
       }
     }
 
     // Special case: the cursor is on an alias, prefer other results.
if (const syntax::Token *NearbyIdent =
         return {*std::move(Macro)};
       }
       ASTResults = locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
                                      *MainFilePath, Index, NodeKind);
       if (!ASTResults.empty()) {
         log("Found definition heuristically using nearby identifier {0}",
             NearbyIdent->text(SM));
         return ASTResults;
-      } else {
-        vlog("No definition found using nearby identifier {0} at {1}",
-             Word->Text, Word->Location.printToString(SM));
       }
+      vlog("No definition found using nearby identifier {0} at {1}", Word->Text,
+           Word->Location.printToString(SM));
     }
     // No nearby word, or it didn't refer to anything either. Try the index.
     auto TextualResults =
         locateSymbolTextually(*Word, AST, Index, *MainFilePath, NodeKind);
     if (!TextualResults.empty())
       return TextualResults;
   }
 
for (const auto &Ref : MainFileRefs) {
       if (Ref.Role & static_cast<unsigned>(index::SymbolRole::Definition))
         Result.Attributes |=
             ReferencesResult::Definition | ReferencesResult::Declaration;
       Results.References.push_back(std::move(Result));
     }
     // Add decl/def of overridding methods.
     if (Index && Results.References.size() <= Limit &&
         !OverriddenBy.Subjects.empty())
-      Index->relations(
-          OverriddenBy, [&](const SymbolID &Subject, const Symbol &Object) {
-            const auto LSPLocDecl =
-                toLSPLocation(Object.CanonicalDeclaration, *MainFilePath);
-            const auto LSPLocDef =
-                toLSPLocation(Object.Definition, *MainFilePath);
-            if (LSPLocDecl && LSPLocDecl != LSPLocDef) {
-              ReferencesResult::Reference Result;
-              Result.Loc = std::move(*LSPLocDecl);
-              Result.Attributes =
-                  ReferencesResult::Declaration | ReferencesResult::Override;
-              Results.References.push_back(std::move(Result));
-            }
-            if (LSPLocDef) {
-              ReferencesResult::Reference Result;
-              Result.Loc = std::move(*LSPLocDef);
-              Result.Attributes = ReferencesResult::Declaration |
-                                  ReferencesResult::Definition |
-                                  ReferencesResult::Override;
-              Results.References.push_back(std::move(Result));
-            }
-          });
+      Index->relations(OverriddenBy, [&](const SymbolID &Subject,
+                                         const Symbol &Object) {
+        const auto LSPLocDef = toLSPLocation(Object.Definition, *MainFilePath);
 
     if (Index && Results.References.size() <= Limit) {
       for (const Decl *D : Decls) {
         // Not all symbols can be referenced from outside (e.g.
         // function-locals).
         // TODO: we could skip TU-scoped symbols here (e.g. static functions) if
         // we know this file isn't a header. The details might be tricky.
         if (D->getParentFunctionOrMethod())

clang-tools-extra/clangd/index/Serialization.cpp

case IndexFileFormat::YAML:
     break;
   }
   return OS;
 }
 
 llvm::Expected<IndexFileIn> readIndexFile(llvm::StringRef Data) {
   if (Data.startswith("RIFF")) {
     return readRIFF(Data);
-  } else if (auto YAMLContents = readYAML(Data)) {
+  }
+  if (auto YAMLContents = readYAML(Data)) {
     return std::move(*YAMLContents);
   } else {
     return error("Not a RIFF file and failed to parse as YAML: {0}",
                  YAMLContents.takeError());
   }
 }
 
 std::unique_ptr<SymbolIndex> loadIndex(llvm::StringRef SymbolFilename,

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp

if (Node == nullptr)
     return false;
 
   // If we're looking at a type or NestedNameSpecifier, walk up the tree until
   // we find the "main" node we care about, which would be ElaboratedTypeLoc or
   // DeclRefExpr.
   for (; Node->Parent; Node = Node->Parent) {
     if (Node->ASTNode.get<NestedNameSpecifierLoc>()) {
       continue;
-    } else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
+    }
+    if (auto *T = Node->ASTNode.get<TypeLoc>()) {
       if (T->getAs<ElaboratedTypeLoc>()) {
         break;
       } else if (Node->Parent->ASTNode.get<TypeLoc>() ||
                  Node->Parent->ASTNode.get<NestedNameSpecifierLoc>()) {
         // Node is TypeLoc, but it's parent is either TypeLoc or
         // NestedNameSpecifier. In both cases, we want to go up, to find
         // the outermost TypeLoc.
         continue;

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

 // This is a very naive check (does it end with a return stmt).
 // Doing some rudimentary control flow analysis would cover more cases.
 bool alwaysReturns(const ExtractionZone &EZ) {
   const Stmt *Last = EZ.getLastRootStmt()->ASTNode.get<Stmt>();
   // Unwrap enclosing (unconditional) compound statement.
   while (const auto *CS = llvm::dyn_cast<CompoundStmt>(Last)) {
     if (CS->body_empty())
       return false;
-    else
-      Last = CS->body_back();
   }
   return llvm::isa<ReturnStmt>(Last);
 }
 
 bool ExtractionZone::isRootStmt(const Stmt *S) const {
   return RootStmts.contains(S);
 }