[clangd] Add error() function for creating formatv-style llvm::Errors. NFC

[sammccall]

Summary

This is considerably terser than the makeStringError and friends, and
avoids verbosity cliffs that discourage adding log information.

It follows the syntax used in log/elog/vlog/dlog that have been successful.

The main caveats are:

• it's strictly out-of-place in logger.h, though kind of fits thematically and in implementation
• it claims the "error" identifier, which seems a bit too opinionated to put higher up in llvm

I've updated some users of StringError mostly at random - there are lots
more mechanical changes but I'd like to get this reviewed before making
them all.

[sammccall]

Move a few comments around.

[njames93]

I'm not a huge fan of using error, maybe createError, its used in many other parts of llvm, typically with static linkage.
Its definitely out of place in Logger, but there is no where else better for it to live short of a new file which seems overkill.

[sammccall]

In D83419#2140311, @njames93 wrote:

I'm not a huge fan of using error, maybe createError,
Its definitely out of place in Logger, but there is no where else better for it to live short of a new file which seems overkill.

Yeah, the naming is definitely the hardest part to be sure of here.
It's an extremely commonly used function: 75 references to StringError + another 20 or so references to local helpers.
This puts it in the same class as log (95), vlog (50), elog(85).
It's also similar in that it is cross cutting and has the same format string + args shape (so wrapping affects it similarly).

its used in many other parts of llvm, typically with static linkage.

Yeah, there's some precedent, though not overwhelming consensus. Outside clangd I count 10 createErrors, 7 make_string_errors, 4 errors, 1 makeError, 1 makeStringError, and the canonical makeStringError and make_error.

But I think this is just a place where wider LLVM is making different tradeoffs:

• for good reasons: many errors are diagnostics or don't need to be carefully routed back to a specific request
• for bad reasons: e.g. compliance with the "verb phrase" policy that's poorly observed and itself seems like a sad accident of history: http://lists.llvm.org/pipermail/llvm-dev/2018-May/thread.html#123547
• llvm::Error itself seems like a pretty strong argument that existing practice doesn't guarantee good ergonomics

[hokein]

In D83419#2140573, @sammccall wrote:

In D83419#2140311, @njames93 wrote:

I'm not a huge fan of using error, maybe createError,
Its definitely out of place in Logger, but there is no where else better for it to live short of a new file which seems overkill.

Yeah, the naming is definitely the hardest part to be sure of here.
It's an extremely commonly used function: 75 references to StringError + another 20 or so references to local helpers.
This puts it in the same class as log (95), vlog (50), elog(85).
It's also similar in that it is cross cutting and has the same format string + args shape (so wrapping affects it similarly).

while the name error aligns well with elog/vlog, I'm not in favor of it. I think the key point is that error creates an Error object, and this object must be checked before destructing. error name makes it look like a print function, I'd prefer to add a verb to the name, e.g. makeError

its used in many other parts of llvm, typically with static linkage.

Yeah, there's some precedent, though not overwhelming consensus. Outside clangd I count 10 createErrors, 7 make_string_errors, 4 errors, 1 makeError, 1 makeStringError, and the canonical makeStringError and make_error.

But I think this is just a place where wider LLVM is making different tradeoffs:

• for good reasons: many errors are diagnostics or don't need to be carefully routed back to a specific request
• for bad reasons: e.g. compliance with the "verb phrase" policy that's poorly observed and itself seems like a sad accident of history: http://lists.llvm.org/pipermail/llvm-dev/2018-May/thread.html#123547
• llvm::Error itself seems like a pretty strong argument that existing practice doesn't guarantee good ergonomics

it would be nicer if we could offer this across llvm-projects, but agree that logger.cc seems to be the best place to place it so far.

[sammccall]

Honestly, I really dislike names in the style of makeError/createError.
My favorite name for this would be Error (i.e. a constructor) but that would involve redesigning it (which is a good idea, but an impossible yak-shave).
I guess makeError is still better than the status quo, but not enough to feel motivated to clean this up right now. Maybe someone else wants to pick this up?

I think the key point is that error creates an Error object, and this object must be checked before destructing. error name makes it look like a print function, I'd prefer to add a verb to the name, e.g. makeError

This is exactly backwards IMO - a function used for its value (like this one) doesn't need a verb, a function used for its side-effects (like log) does.
https://swift.org/documentation/api-design-guidelines/#strive-for-fluent-usage seems pretty well-thought-out to me.
LLVM guidelines don't reflect this, but I haven't been able to find anyone making a positive case for them beyond legacy and inertia.

[sammccall]

In D83419#2140889, @sammccall wrote:

Honestly, I really dislike names in the style of makeError/createError.

Sorry, I should give some reasons here:

• not discoverable (error is marginally better)
• emphasis on allocation rather than how you're using the error (error is much better)
• forms part of a cluster of hard to remember names createX vs makeX vs make_x (error is much better)
• longer, both in characters and words (error is better)

It's just an aesthetic irritation but really this is just an aesthetic cleanup.

[hokein]

Sorry, I should give some reasons here:

These are sensible reasons. My only (not a strong) concern is that "error" is quite special, we need to be careful to choose it -- note that there is an error function in glibc which is used for error-reporting.

maybe there are other better names (stringError?) other than createError/makeError.

I guess makeError is still better than the status quo, but not enough to feel motivated to clean this up right now. Maybe someone else wants to pick this up?

unless you strongly insist on error name, I'm happy to help with (looks like it's just a low-hanging rename fruit). I think this is a nice cleanup, we should make it happen (either using error or other names).

[sammccall]

In D83419#2141433, @hokein wrote:

Sorry, I should give some reasons here:

These are sensible reasons. My only (not a strong) concern is that "error" is quite special, we need to be careful to choose it -- note that there is an error function in glibc which is used for error-reporting.

Yeah. This is also true of log which is is a c standard library function. I've seen slightly funny diagnostics when the header is missing, but no worse than that. It's not possible to actually call the wrong function here - neither of the first two parameters of glibc error() can be a string.

maybe there are other better names (stringError?) other than createError/makeError.

Maybe, it's hard to know what's important enough to include in the name. Neither the fact that the implementation stores a string, or that we're allocating an object seem to qualify to me.
Maybe something to do with the formatting like errorV or fmtError or something but they seem pretty ugly. The difficulty of being part of the log family, which doesn't have suffixes...

I guess makeError is still better than the status quo, but not enough to feel motivated to clean this up right now. Maybe someone else wants to pick this up?

unless you strongly insist on error name, I'm happy to help with (looks like it's just a low-hanging rename fruit). I think this is a nice cleanup, we should make it happen (either using error or other names).

Sure. I'm probably letting the perfect be the enemy of the good here, still sad about the json::Value::asInt() -> json::Value::getAsInt() thing...

[hokein]

anyway, I'm fine with this change, feel free to submit it as-is (or do any refinements), don't really want the naming to block this.

clang-tools-extra/clangd/ClangdLSPServer.cpp

if (auto Draft = DraftMgr.getDraft(It.first())) {
         ++InvalidFileCount;
         LastInvalidFile = It.first();
       }
     }
   }
   if (!InvalidFileCount)
     return llvm::Error::success();
   if (InvalidFileCount == 1)
-    return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                   "File must be saved first: " +
-                                       LastInvalidFile);
-  return llvm::createStringError(
-      llvm::inconvertibleErrorCode(),
-      "Files must be saved first: " + LastInvalidFile + " (and " +
-          llvm::to_string(InvalidFileCount - 1) + " others)");
+    return error("File must be saved first: {0}", LastInvalidFile);
+  return error("Files must be saved first: {0} (and {1} others)",
+               LastInvalidFile, InvalidFileCount - 1);
 }
 
 } // namespace
 
 // MessageHandler dispatches incoming LSP messages.
 // It handles cross-cutting concerns:
 //  - serializes/deserializes protocol objects to JSON
 //  - logging of inbound messages
int ID;
       if (ReplyCallbacks.size() > MaxReplayCallbacks) {
         elog("more than {0} outstanding LSP calls, forgetting about {1}",
              MaxReplayCallbacks, ReplyCallbacks.front().first);
         OldestCB = std::move(ReplyCallbacks.front());
         ReplyCallbacks.pop_front();
       }
     }
     if (OldestCB)
-      OldestCB->second(llvm::createStringError(
-          llvm::inconvertibleErrorCode(),
-          llvm::formatv("failed to receive a client reply for request ({0})",
-                        OldestCB->first)));
+      OldestCB->second(
+          error("failed to receive a client reply for request ({0})",
+                OldestCB->first));
     return ID;
   }
 
   // Bind an LSP method name to a notification.
   template <typename Param>
   void bind(const char *Method,
             void (ClangdLSPServer::*Handler)(const Param &)) {
     Notifications[Method] = [Method, Handler,

 
 // sync is a clangd extension: it blocks until all background work completes.
 // It blocks the calling thread, so no messages are processed until it returns!
 void ClangdLSPServer::onSync(const NoParams &Params,
                              Callback<std::nullptr_t> Reply) {
   if (Server->blockUntilIdleForTest(/*TimeoutSeconds=*/60))
     Reply(nullptr);
   else
-    Reply(llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                  "Not idle after a minute"));
+    Reply(error("Not idle after a minute"));
 }
 
 void ClangdLSPServer::onDocumentDidOpen(
     const DidOpenTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
 
   const std::string &Contents = Params.textDocument.text;
 
call<ApplyWorkspaceEditResponse>(
         [Reply = std::move(Reply), SuccessMessage = std::move(SuccessMessage)](
             llvm::Expected<ApplyWorkspaceEditResponse> Response) mutable {
           if (!Response)
             return Reply(Response.takeError());
           if (!Response->applied) {
             std::string Reason = Response->failureReason
                                      ? *Response->failureReason
                                      : "unknown reason";
-            return Reply(llvm::createStringError(
-                llvm::inconvertibleErrorCode(),
-                ("edits were not applied: " + Reason).c_str()));
+            return Reply(error("edits were not applied: {0}", Reason));
           }
           return Reply(SuccessMessage);
         });
   };
 
   if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND &&
       Params.workspaceEdit) {
     // The flow for "apply-fix" :
     // 1. We publish a diagnostic, including fixits
     // 2. The user clicks on the diagnostic, the editor asks us for code actions
     // 3. We send code actions, with the fixit embedded as context
     // 4. The user selects the fixit, the editor asks us to apply it
     // 5. We unwrap the changes and send them back to the editor
     // 6. The editor applies the changes (applyEdit), and sends us a reply
     // 7. We unwrap the reply and send a reply to the editor.
     ApplyEdit(*Params.workspaceEdit, "Fix applied.", std::move(Reply));
   } else if (Params.command == ExecuteCommandParams::CLANGD_APPLY_TWEAK &&
              Params.tweakArgs) {
     auto Code = DraftMgr.getDraft(Params.tweakArgs->file.file());
     if (!Code)
-      return Reply(llvm::createStringError(
-          llvm::inconvertibleErrorCode(),
-          "trying to apply a code action for a non-added file"));
+      return Reply(error("trying to apply a code action for a non-added file"));
 
     auto Action = [this, ApplyEdit, Reply = std::move(Reply),
                    File = Params.tweakArgs->file, Code = std::move(*Code)](
                       llvm::Expected<Tweak::Effect> R) mutable {
       if (!R)
         return Reply(R.takeError());
 
       assert(R->ShowMessage ||

clang-tools-extra/clangd/ClangdServer.cpp

void ClangdServer::signatureHelp(PathRef File, Position Pos,
 
   auto Action = [Pos, File = File.str(), CB = std::move(CB),
                  this](llvm::Expected<InputsAndPreamble> IP) mutable {
     if (!IP)
       return CB(IP.takeError());
 
     const auto *PreambleData = IP->Preamble;
     if (!PreambleData)
-      return CB(llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                        "Failed to parse includes"));
+      return CB(error("Failed to parse includes"));
 
     ParseInputs ParseInput{IP->Command, &TFS, IP->Contents.str()};
     ParseInput.Index = Index;
     ParseInput.Opts.BuildRecoveryAST = BuildRecoveryAST;
     ParseInput.Opts.PreserveRecoveryASTType = PreserveRecoveryASTType;
     CB(clangd::signatureHelp(File, Pos, *PreambleData, ParseInput));
   };
 

clang-tools-extra/clangd/CodeComplete.cpp

auto Inserted = [&](llvm::StringRef Header)
           URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
       if (!ResolvedDeclaring)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
       auto Spelled = Includes.calculateIncludePath(*ResolvedInserted, FileName);
       if (!Spelled)
-        return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                       "Header not on include path");
+        return error("Header not on include path");
       return std::make_pair(
           std::move(*Spelled),
           Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
     };
     bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
     // Calculate include paths and edits for all possible headers.
     for (const auto &Inc : C.RankedIncludeHeaders) {
       if (auto ToInclude = Inserted(Inc)) {

clang-tools-extra/clangd/DraftStore.cpp

 
 llvm::Expected<DraftStore::Draft> DraftStore::updateDraft(
     PathRef File, llvm::Optional<int64_t> Version,
     llvm::ArrayRef<TextDocumentContentChangeEvent> Changes) {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   auto EntryIt = Drafts.find(File);
   if (EntryIt == Drafts.end()) {
-    return llvm::make_error<llvm::StringError>(
-        "Trying to do incremental update on non-added document: " + File,
-        llvm::errc::invalid_argument);
+    return error(llvm::errc::invalid_argument,
+                 "Trying to do incremental update on non-added document: {0}",
+                 File);
   }
   Draft &D = EntryIt->second;
   std::string Contents = EntryIt->second.Contents;
 
   for (const TextDocumentContentChangeEvent &Change : Changes) {
     if (!Change.range) {
       Contents = Change.text;
       continue;
     }
 
     const Position &Start = Change.range->start;
     llvm::Expected<size_t> StartIndex =
         positionToOffset(Contents, Start, false);
     if (!StartIndex)
       return StartIndex.takeError();
 
     const Position &End = Change.range->end;
     llvm::Expected<size_t> EndIndex = positionToOffset(Contents, End, false);
     if (!EndIndex)
       return EndIndex.takeError();
 
     if (*EndIndex < *StartIndex)
-      return llvm::make_error<llvm::StringError>(
-          llvm::formatv(
-              "Range's end position ({0}) is before start position ({1})", End,
-              Start),
-          llvm::errc::invalid_argument);
+      return error(llvm::errc::invalid_argument,
+                   "Range's end position ({0}) is before start position ({1})",
+                   End, Start);
 
     // Since the range length between two LSP positions is dependent on the
     // contents of the buffer we compute the range length between the start and
     // end position ourselves and compare it to the range length of the LSP
     // message to verify the buffers of the client and server are in sync.
 
     // EndIndex and StartIndex are in bytes, but Change.rangeLength is in UTF-16
     // code units.
     ssize_t ComputedRangeLength =
         lspLength(Contents.substr(*StartIndex, *EndIndex - *StartIndex));
 
     if (Change.rangeLength && ComputedRangeLength != *Change.rangeLength)
-      return llvm::make_error<llvm::StringError>(
-          llvm::formatv("Change's rangeLength ({0}) doesn't match the "
-                        "computed range length ({1}).",
-                        *Change.rangeLength, ComputedRangeLength),
-          llvm::errc::invalid_argument);
+      return error(llvm::errc::invalid_argument,
+                   "Change's rangeLength ({0}) doesn't match the "
+                   *Change.rangeLength, ComputedRangeLength);
 
     std::string NewContents;
     NewContents.reserve(*StartIndex + Change.text.length() +
                         (Contents.length() - *EndIndex));
 
     NewContents = Contents.substr(0, *StartIndex);
     NewContents += Change.text;
     NewContents += Contents.substr(*EndIndex);

clang-tools-extra/clangd/JSONTransport.cpp

llvm::json::Object encodeError(llvm::Error E) {
 
   return llvm::json::Object{
       {"message", std::move(Message)},
       {"code", int64_t(Code)},
   };
 }
 
 llvm::Error decodeError(const llvm::json::Object &O) {
-  std::string Msg =
-      std::string(O.getString("message").getValueOr("Unspecified error"));
+  llvm::StringRef Msg = O.getString("message").getValueOr("Unspecified error");
   if (auto Code = O.getInteger("code"))
-    return llvm::make_error<LSPError>(std::move(Msg), ErrorCode(*Code));
-  return llvm::make_error<llvm::StringError>(std::move(Msg),
-                                             llvm::inconvertibleErrorCode());
+    return llvm::make_error<LSPError>(Msg.str(), ErrorCode(*Code));
+  return error(Msg.str());
 }
 
 class JSONTransport : public Transport {
 public:
   JSONTransport(std::FILE *In, llvm::raw_ostream &Out,
                 llvm::raw_ostream *InMirror, bool Pretty, JSONStreamStyle Style)
       : In(In), Out(Out), InMirror(InMirror ? *InMirror : llvm::nulls()),
         Pretty(Pretty), Style(Style) {}

clang-tools-extra/clangd/PathMapping.cpp

 //===--- PathMapping.cpp - apply path mappings to LSP messages -===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "PathMapping.h"
 #include "Transport.h"
 #include "URI.h"
+#include "support/Logger.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Errno.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include <algorithm>
 #include <tuple>
 
llvm::Expected<std::string> parsePath(llvm::StringRef Path) {
   if (path::is_absolute(Path, path::Style::posix)) {
     return std::string(Path);
   } else 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 llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                 "Path not absolute: " + Path);
+  return error("Path not absolute: {0}", Path);
 }
 
 } // namespace
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PathMapping &M) {
   return OS << M.ClientPath << "=" << M.ServerPath;
 }
 
 llvm::Expected<PathMappings>
 parsePathMappings(llvm::StringRef RawPathMappings) {
   llvm::StringRef ClientPath, ServerPath, PathPair, Rest = RawPathMappings;
   PathMappings ParsedMappings;
   while (!Rest.empty()) {
     std::tie(PathPair, Rest) = Rest.split(",");
     std::tie(ClientPath, ServerPath) = PathPair.split("=");
     if (ClientPath.empty() || ServerPath.empty())
-      return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                     "Not a valid path mapping pair: " +
-                                         PathPair);
+      return error("Not a valid path mapping pair: {0}", PathPair);
     llvm::Expected<std::string> ParsedClientPath = parsePath(ClientPath);
     if (!ParsedClientPath)
       return ParsedClientPath.takeError();
     llvm::Expected<std::string> ParsedServerPath = parsePath(ServerPath);
     if (!ParsedServerPath)
       return ParsedServerPath.takeError();
     ParsedMappings.push_back(
         {std::move(*ParsedClientPath), std::move(*ParsedServerPath)});

clang-tools-extra/clangd/RIFF.cpp

 //===--- RIFF.cpp - Binary container file format --------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "RIFF.h"
+#include "support/Logger.h"
 #include "llvm/Support/Endian.h"
-#include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 namespace riff {
 
-static llvm::Error makeError(const llvm::Twine &Msg) {
-  return llvm::make_error<llvm::StringError>(Msg,
-                                             llvm::inconvertibleErrorCode());
-}
-
 llvm::Expected<Chunk> readChunk(llvm::StringRef &Stream) {
   if (Stream.size() < 8)
-    return makeError("incomplete chunk header: " + llvm::Twine(Stream.size()) +
-                     " bytes available");
+    return error("incomplete chunk header: {0} bytes available", Stream.size());
   Chunk C;
   std::copy(Stream.begin(), Stream.begin() + 4, C.ID.begin());
   Stream = Stream.drop_front(4);
   uint32_t Len = llvm::support::endian::read32le(Stream.take_front(4).begin());
   Stream = Stream.drop_front(4);
   if (Stream.size() < Len)
-    return makeError("truncated chunk: want " + llvm::Twine(Len) + ", got " +
-                     llvm::Twine(Stream.size()));
+    return error("truncated chunk: want {0}, got {1}", Len, Stream.size());
   C.Data = Stream.take_front(Len);
   Stream = Stream.drop_front(Len);
   if ((Len % 2) && !Stream.empty()) { // Skip padding byte.
     if (Stream.front())
-      return makeError("nonzero padding byte");
+      return error("nonzero padding byte");
     Stream = Stream.drop_front();
   }
   return std::move(C);
 }
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Chunk &C) {
   OS.write(C.ID.data(), C.ID.size());
   char Size[4];
   llvm::support::endian::write32le(Size, C.Data.size());
   OS.write(Size, sizeof(Size));
   OS << C.Data;
   if (C.Data.size() % 2)
     OS.write(0);
   return OS;
 }
 
 llvm::Expected<File> readFile(llvm::StringRef Stream) {
   auto RIFF = readChunk(Stream);
   if (!RIFF)
     return RIFF.takeError();
   if (RIFF->ID != fourCC("RIFF"))
-    return makeError("not a RIFF container: root is " + fourCCStr(RIFF->ID));
+    return error("not a RIFF container: root is {0}", fourCCStr(RIFF->ID));
   if (RIFF->Data.size() < 4)
-    return makeError("RIFF chunk too short");
+    return error("RIFF chunk too short");
   File F;
   std::copy(RIFF->Data.begin(), RIFF->Data.begin() + 4, F.Type.begin());
   for (llvm::StringRef Body = RIFF->Data.drop_front(4); !Body.empty();)
     if (auto Chunk = readChunk(Body)) {
       F.Chunks.push_back(*Chunk);
     } else
       return Chunk.takeError();
   return std::move(F);

clang-tools-extra/clangd/TUScheduler.cpp

if (!AST) {
       }
       AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
     }
     // Make sure we put the AST back into the LRU cache.
     auto _ = llvm::make_scope_exit(
         [&AST, this]() { IdleASTs.put(this, std::move(*AST)); });
     // Run the user-provided action.
     if (!*AST)
-      return Action(llvm::make_error<llvm::StringError>(
-          "invalid AST", llvm::errc::invalid_argument));
+      return Action(error(llvm::errc::invalid_argument, "invalid AST"));
     vlog("ASTWorker running {0} on version {2} of {1}", Name, FileName,
          FileInputs.Version);
     Action(InputsAndAST{FileInputs, **AST});
   };
   startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation);
 }
 
 void PreambleThread::build(Request Req) {

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

 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
-llvm::Error makeError(const llvm::Twine &Msg) {
-  return llvm::make_error<llvm::StringError>(Msg,
-                                             llvm::inconvertibleErrorCode());
-}
 
 // IO PRIMITIVES
 // We use little-endian 32 bit ints, sometimes with variable-length encoding.
 //
 // Variable-length int encoding (varint) uses the bottom 7 bits of each byte
 // to encode the number, and the top bit to indicate whether more bytes follow.
 // e.g. 9a 2f means [0x1a and keep reading, 0x2f and stop].
 // This represents 0x1a | 0x2f<<7 = 6042.
struct StringTableIn {
   llvm::BumpPtrAllocator Arena;
   std::vector<llvm::StringRef> Strings;
 };
 
 llvm::Expected<StringTableIn> readStringTable(llvm::StringRef Data) {
   Reader R(Data);
   size_t UncompressedSize = R.consume32();
   if (R.err())
-    return makeError("Truncated string table");
+    return error("Truncated string table");
 
   llvm::StringRef Uncompressed;
   llvm::SmallString<1> UncompressedStorage;
   if (UncompressedSize == 0) // No compression
     Uncompressed = R.rest();
   else {
     if (llvm::Error E = llvm::zlib::uncompress(R.rest(), UncompressedStorage,
                                                UncompressedSize))
       return std::move(E);
     Uncompressed = UncompressedStorage;
   }
 
   StringTableIn Table;
   llvm::StringSaver Saver(Table.Arena);
   R = Reader(Uncompressed);
   for (Reader R(Uncompressed); !R.eof();) {
     auto Len = R.rest().find(0);
     if (Len == llvm::StringRef::npos)
-      return makeError("Bad string table: not null terminated");
+      return error("Bad string table: not null terminated");
     Table.Strings.push_back(Saver.save(R.consume(Len)));
     R.consume8();
   }
   if (R.err())
-    return makeError("Truncated string table");
+    return error("Truncated string table");
   return std::move(Table);
 }
 
 // SYMBOL ENCODING
 // Each field of clangd::Symbol is encoded in turn (see implementation).
 //  - StringRef fields encode as varint (index into the string table)
 //  - enums encode as the underlying type
 //  - most numbers encode as varint

 // data. Later we may want to support some backward compatibility.
 constexpr static uint32_t Version = 13;
 
 llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data) {
   auto RIFF = riff::readFile(Data);
   if (!RIFF)
     return RIFF.takeError();
   if (RIFF->Type != riff::fourCC("CdIx"))
-    return makeError("wrong RIFF filetype: " + riff::fourCCStr(RIFF->Type));
+    return error("wrong RIFF filetype: {0}", riff::fourCCStr(RIFF->Type));
   llvm::StringMap<llvm::StringRef> Chunks;
   for (const auto &Chunk : RIFF->Chunks)
     Chunks.try_emplace(llvm::StringRef(Chunk.ID.data(), Chunk.ID.size()),
                        Chunk.Data);
 
   if (!Chunks.count("meta"))
-    return makeError("missing meta chunk");
+    return error("missing meta chunk");
   Reader Meta(Chunks.lookup("meta"));
   auto SeenVersion = Meta.consume32();
   if (SeenVersion != Version)
-    return makeError("wrong version: want " + llvm::Twine(Version) + ", got " +
-                     llvm::Twine(SeenVersion));
+    return error("wrong version: want {0}, got {1}", Version, SeenVersion);
 
   // meta chunk is checked above, as we prefer the "version mismatch" error.
   for (llvm::StringRef RequiredChunk : {"stri"})
     if (!Chunks.count(RequiredChunk))
-      return makeError("missing required chunk " + RequiredChunk);
+      return error("missing required chunk {0}", RequiredChunk);
 
   auto Strings = readStringTable(Chunks.lookup("stri"));
   if (!Strings)
     return Strings.takeError();
 
   IndexFileIn Result;
   if (Chunks.count("srcs")) {
     Reader SrcsReader(Chunks.lookup("srcs"));
     Result.Sources.emplace();
     while (!SrcsReader.eof()) {
       auto IGN = readIncludeGraphNode(SrcsReader, Strings->Strings);
       auto Entry = Result.Sources->try_emplace(IGN.URI).first;
       Entry->getValue() = std::move(IGN);
       // We change all the strings inside the structure to point at the keys in
       // the map, since it is the only copy of the string that's going to live.
       Entry->getValue().URI = Entry->getKey();
       for (auto &Include : Entry->getValue().DirectIncludes)
         Include = Result.Sources->try_emplace(Include).first->getKey();
     }
     if (SrcsReader.err())
-      return makeError("malformed or truncated include uri");
+      return error("malformed or truncated include uri");
   }
 
   if (Chunks.count("symb")) {
     Reader SymbolReader(Chunks.lookup("symb"));
     SymbolSlab::Builder Symbols;
     while (!SymbolReader.eof())
       Symbols.insert(readSymbol(SymbolReader, Strings->Strings));
     if (SymbolReader.err())
-      return makeError("malformed or truncated symbol");
+      return error("malformed or truncated symbol");
     Result.Symbols = std::move(Symbols).build();
   }
   if (Chunks.count("refs")) {
     Reader RefsReader(Chunks.lookup("refs"));
     RefSlab::Builder Refs;
     while (!RefsReader.eof()) {
       auto RefsBundle = readRefs(RefsReader, Strings->Strings);
       for (const auto &Ref : RefsBundle.second) // FIXME: bulk insert?
         Refs.insert(RefsBundle.first, Ref);
     }
     if (RefsReader.err())
-      return makeError("malformed or truncated refs");
+      return error("malformed or truncated refs");
     Result.Refs = std::move(Refs).build();
   }
   if (Chunks.count("rela")) {
     Reader RelationsReader(Chunks.lookup("rela"));
     RelationSlab::Builder Relations;
     while (!RelationsReader.eof()) {
       auto Relation = readRelation(RelationsReader);
       Relations.insert(Relation);
     }
     if (RelationsReader.err())
-      return makeError("malformed or truncated relations");
+      return error("malformed or truncated relations");
     Result.Relations = std::move(Relations).build();
   }
   if (Chunks.count("cmdl")) {
     Reader CmdReader(Chunks.lookup("cmdl"));
     if (CmdReader.err())
-      return makeError("malformed or truncated commandline section");
+      return error("malformed or truncated commandline section");
     InternedCompileCommand Cmd =
         readCompileCommand(CmdReader, Strings->Strings);
     Result.Cmd.emplace();
     Result.Cmd->Directory = std::string(Cmd.Directory);
     Result.Cmd->CommandLine.reserve(Cmd.CommandLine.size());
     for (llvm::StringRef C : Cmd.CommandLine)
       Result.Cmd->CommandLine.emplace_back(C);
   }

 }
 
 llvm::Expected<IndexFileIn> readIndexFile(llvm::StringRef Data) {
   if (Data.startswith("RIFF")) {
     return readRIFF(Data);
   } else if (auto YAMLContents = readYAML(Data)) {
     return std::move(*YAMLContents);
   } else {
-    return makeError("Not a RIFF file and failed to parse as YAML: " +
-                     llvm::toString(YAMLContents.takeError()));
+    return error("Not a RIFF file and failed to parse as YAML: {0}",
+                 YAMLContents.takeError());
   }
 }
 
 std::unique_ptr<SymbolIndex> loadIndex(llvm::StringRef SymbolFilename,
                                        bool UseDex) {
   trace::Span OverallTracer("LoadIndex");
   auto Buffer = llvm::MemoryBuffer::getFile(SymbolFilename);
   if (!Buffer) {

clang-tools-extra/clangd/support/Logger.h

 template <typename T> T &&wrap(T &&V) { return std::forward<T>(V); }
 inline decltype(fmt_consume(llvm::Error::success())) wrap(llvm::Error &&V) {
   return fmt_consume(std::move(V));
 }
 template <typename... Ts>
 void log(Logger::Level L, const char *Fmt, Ts &&... Vals) {
   detail::log(L, llvm::formatv(Fmt, detail::wrap(std::forward<Ts>(Vals))...));
 }
+
+llvm::Error error(std::error_code, std::string &&);
 } // namespace detail
 
 // Clangd logging functions write to a global logger set by LoggingSession.
 // If no logger is registered, writes to llvm::errs().
 // All accept llvm::formatv()-style arguments, e.g. log("Text={0}", Text).
 
 // elog() is used for "loud" errors and warnings.
 // This level is often visible to users.
 template <typename... Ts> void elog(const char *Fmt, Ts &&... Vals) {
   detail::log(Logger::Error, Fmt, std::forward<Ts>(Vals)...);
 }
 // log() is used for information important to understand a clangd session.
 // e.g. the names of LSP messages sent are logged at this level.
 // This level could be enabled in production builds to allow later inspection.
 template <typename... Ts> void log(const char *Fmt, Ts &&... Vals) {
   detail::log(Logger::Info, Fmt, std::forward<Ts>(Vals)...);
 }
 // vlog() is used for details often needed for debugging clangd sessions.
 // This level would typically be enabled for clangd developers.
 template <typename... Ts> void vlog(const char *Fmt, Ts &&... Vals) {
   detail::log(Logger::Verbose, Fmt, std::forward<Ts>(Vals)...);
 }
+// error() constructs an llvm::Error object, using formatv()-style arguments.
+// It is not automatically logged! (This function is a little out of place).
+// The error simply embeds the message string.
+template <typename... Ts>
+llvm::Error error(std::error_code EC, const char *Fmt, Ts &&... Vals) {
+  // We must render the formatv_object eagerly, while references are valid.
+  return detail::error(
+      EC, llvm::formatv(Fmt, detail::wrap(std::forward<Ts>(Vals))...).str());
+}
+// Overload with no error_code conversion, the error will be inconvertible.
+template <typename... Ts> llvm::Error error(const char *Fmt, Ts &&... Vals) {
+  return detail::error(
+      llvm::inconvertibleErrorCode(),
+      llvm::formatv(Fmt, detail::wrap(std::forward<Ts>(Vals))...).str());
+}
+// Overload to avoid formatv complexity for simple strings.
+inline llvm::Error error(std::error_code EC, std::string Msg) {
+  return detail::error(EC, std::move(Msg));
+}
+// Overload for simple strings with no error_code conversion.
+inline llvm::Error error(std::string Msg) {
+  return detail::error(llvm::inconvertibleErrorCode(), std::move(Msg));
+}
+
 // dlog only logs if --debug was passed, or --debug_only=Basename.
 // This level would be enabled in a targeted way when debugging.
 #define dlog(...)                                                              \
   DEBUG_WITH_TYPE(::clang::clangd::detail::debugType(__FILE__),                \
                   ::clang::clangd::detail::log(Logger::Debug, __VA_ARGS__))
 
 /// Only one LoggingSession can be active at a time.
 class LoggingSession {

clang-tools-extra/clangd/support/Logger.cpp

 //===--- Logger.cpp - Logger interface for clangd -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "llvm/Support/Chrono.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <mutex>
 
 namespace clang {
 namespace clangd {
 
 namespace {
void StreamLogger::log(Logger::Level Level,
   llvm::sys::TimePoint<> Timestamp = std::chrono::system_clock::now();
   trace::log(Message);
   std::lock_guard<std::mutex> Guard(StreamMutex);
   Logs << llvm::formatv("{0}[{1:%H:%M:%S.%L}] {2}\n", indicator(Level),
                         Timestamp, Message);
   Logs.flush();
 }
 
+namespace {
+// Like llvm::StringError but with fewer options and no gratuitous copies.
+class SimpleStringError : public llvm::ErrorInfo<SimpleStringError> {
+  std::error_code EC;
+  std::string Message;
+
+public:
+  SimpleStringError(std::error_code EC, std::string &&Message)
+      : EC(EC), Message(std::move(Message)) {}
+  void log(llvm::raw_ostream &OS) const override { OS << Message; }
+  std::string message() const override { return Message; }
+  std::error_code convertToErrorCode() const override { return EC; }
+  static char ID;
+};
+char SimpleStringError::ID;
+
+} // namespace
+
+llvm::Error detail::error(std::error_code EC, std::string &&Msg) {
+  return llvm::make_error<SimpleStringError>(EC, std::move(Msg));
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   FuzzyMatchTests.cpp
   GlobalCompilationDatabaseTests.cpp
   HeadersTests.cpp
   HeaderSourceSwitchTests.cpp
   HoverTests.cpp
   IndexActionTests.cpp
   IndexTests.cpp
   JSONTransportTests.cpp
+  LoggerTests.cpp
   LSPClient.cpp
   ModulesTests.cpp
   ParsedASTTests.cpp
   PathMappingTests.cpp
   PreambleTests.cpp
   PrintASTTests.cpp
   QualityTests.cpp
   RenameTests.cpp

clang-tools-extra/clangd/unittests/LoggerTests.cpp

+//===-- LoggerTests.cpp ---------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "support/Logger.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(ErrorTest, Overloads) {
+  EXPECT_EQ("foo", llvm::toString(error("foo")));
+  // Inconvertible to error code when none is specified.
+  // Don't actually try to convert, it'll crash.
+  handleAllErrors(error("foo"), [&](const llvm::ErrorInfoBase &EI) {
+    EXPECT_EQ(llvm::inconvertibleErrorCode(), EI.convertToErrorCode());
+  });
+
+  EXPECT_EQ("foo 42", llvm::toString(error("foo {0}", 42)));
+  handleAllErrors(error("foo {0}", 42), [&](const llvm::ErrorInfoBase &EI) {
+    EXPECT_EQ(llvm::inconvertibleErrorCode(), EI.convertToErrorCode());
+  });
+
+  EXPECT_EQ("foo", llvm::toString(error(llvm::errc::invalid_argument, "foo")));
+  EXPECT_EQ(llvm::errc::invalid_argument,
+            llvm::errorToErrorCode(error(llvm::errc::invalid_argument, "foo")));
+
+  EXPECT_EQ("foo 42",
+            llvm::toString(error(llvm::errc::invalid_argument, "foo {0}", 42)));
+  EXPECT_EQ(llvm::errc::invalid_argument,
+            llvm::errorToErrorCode(
+                error(llvm::errc::invalid_argument, "foo {0}", 42)));
+}
+
+TEST(ErrorTest, Lifetimes) {
+  llvm::Optional<llvm::Error> Err;
+  {
+    // Check the error contains the value when error() was called.
+    std::string S = "hello, world";
+    Err = error("S={0}", llvm::StringRef(S));
+    S = "garbage";
+  }
+  EXPECT_EQ("S=hello, world", llvm::toString(std::move(*Err)));
+}
+
+TEST(ErrorTest, ConsumeError) {
+  llvm::Error Foo = error("foo");
+  llvm::Error Bar = error("bar: {0}", std::move(Foo));
+  EXPECT_EQ("bar: foo", llvm::toString(std::move(Bar)));
+  // No assert for unchecked Foo.
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang