caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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.

Reviewers: kbobyrev, hokein

Subscribers: mgorny, ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D83419
This commit is contained in:
Sam McCall
2020-07-08 21:49:38 +02:00
parent 09b8871f8d
commit 30667c967d
13 changed files with 160 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
clang-tools-extra/clangd/ClangdLSPServer.cpp
View File

@@ -147,13 +147,9 @@ llvm::Error validateEdits(const DraftStore &DraftMgr, const FileEdits &FE) {
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
@@ -284,10 +280,9 @@ public:
}
}
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;
}
@@ -661,8 +656,7 @@ void ClangdLSPServer::onSync(const NoParams &Params,
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(
@@ -729,9 +723,7 @@ void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params,
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);
});
@@ -752,9 +744,7 @@ void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params,
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)](

3
clang-tools-extra/clangd/ClangdServer.cpp
View File

@@ -342,8 +342,7 @@ void ClangdServer::signatureHelp(PathRef File, Position Pos,
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;

3
clang-tools-extra/clangd/CodeComplete.cpp
View File

@@ -333,8 +333,7 @@ struct CodeCompletionBuilder {
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));

23
clang-tools-extra/clangd/DraftStore.cpp
View File

@@ -64,9 +64,9 @@ llvm::Expected<DraftStore::Draft> DraftStore::updateDraft(
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;
@@ -89,11 +89,9 @@ llvm::Expected<DraftStore::Draft> DraftStore::updateDraft(
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
@@ -106,11 +104,10 @@ llvm::Expected<DraftStore::Draft> DraftStore::updateDraft(
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 "
"computed range length ({1}).",
*Change.rangeLength, ComputedRangeLength);
std::string NewContents;
NewContents.reserve(*StartIndex + Change.text.length() +

8
clang-tools-extra/clangd/JSONTransport.cpp
View File

@@ -51,12 +51,10 @@ llvm::json::Object encodeError(llvm::Error E) {
}
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 {

8
clang-tools-extra/clangd/PathMapping.cpp
View File

@@ -8,6 +8,7 @@
#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"
@@ -156,8 +157,7 @@ llvm::Expected<std::string> parsePath(llvm::StringRef Path) {
Converted = "/" + Converted;
return Converted;
}
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"Path not absolute: " + Path);
return error("Path not absolute: {0}", Path);
}
} // namespace
@@ -174,9 +174,7 @@ parsePathMappings(llvm::StringRef RawPathMappings) {
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();

19
clang-tools-extra/clangd/RIFF.cpp
View File

@@ -7,35 +7,28 @@
//===----------------------------------------------------------------------===//
#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);
@@ -57,9 +50,9 @@ llvm::Expected<File> readFile(llvm::StringRef 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();)

3
clang-tools-extra/clangd/TUScheduler.cpp
View File

@@ -717,8 +717,7 @@ void ASTWorker::runWithAST(
[&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});

33
clang-tools-extra/clangd/index/Serialization.cpp
View File

@@ -25,10 +25,6 @@
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.
@@ -199,7 +195,7 @@ 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;
@@ -218,12 +214,12 @@ llvm::Expected<StringTableIn> readStringTable(llvm::StringRef Data) {
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);
}
@@ -426,24 +422,23 @@ llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef 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)
@@ -464,7 +459,7 @@ llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data) {
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")) {
@@ -473,7 +468,7 @@ llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data) {
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")) {
@@ -485,7 +480,7 @@ llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data) {
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")) {
@@ -496,13 +491,13 @@ llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data) {
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();
@@ -660,8 +655,8 @@ llvm::Expected<IndexFileIn> readIndexFile(llvm::StringRef 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());
}
}

23
clang-tools-extra/clangd/support/Logger.cpp
View File

@@ -9,6 +9,7 @@
#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>
@@ -58,5 +59,27 @@ void StreamLogger::log(Logger::Level Level,
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

26
clang-tools-extra/clangd/support/Logger.h
View File

@@ -45,6 +45,8 @@ 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.
@@ -67,6 +69,30 @@ template <typename... Ts> void log(const char *Fmt, Ts &&... Vals) {
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(...) \

1
clang-tools-extra/clangd/unittests/CMakeLists.txt
View File

@@ -62,6 +62,7 @@ add_unittest(ClangdUnitTests ClangdTests
IndexActionTests.cpp
IndexTests.cpp
JSONTransportTests.cpp
LoggerTests.cpp
LSPClient.cpp
ModulesTests.cpp
ParsedASTTests.cpp

62
clang-tools-extra/clangd/unittests/LoggerTests.cpp Normal file
View File

@@ -0,0 +1,62 @@
//===-- 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