From b73e706589d36a11b6b805e9866e006580b34ede Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 1 Jun 2025 20:55:16 +0000 Subject: [PATCH 1/2] libutil: Use `std::shared_ptr` instead of `std::shared_ptr` There's actually no mutation happening so there's no point in using a mutable shared_ptr. Furthermore, this makes it much more evident to the reader that no actual mutation (especially in multithreaded case) is happening. Also get rid of redundant constructor that isn't actually used anywhere other than `Pos::operator std::shared_ptr` which just passes in &*this, (identical to just `this`), which can't be nullptr. --- src/libutil/error.cc | 4 ++-- src/libutil/include/nix/util/error.hh | 10 +++++----- src/libutil/include/nix/util/position.hh | 4 +--- src/libutil/logging.cc | 2 +- src/libutil/position.cc | 14 ++------------ 5 files changed, 11 insertions(+), 23 deletions(-) diff --git a/src/libutil/error.cc b/src/libutil/error.cc index 0ceaa4e76..049555ea3 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -13,7 +13,7 @@ namespace nix { -void BaseError::addTrace(std::shared_ptr && e, HintFmt hint, TracePrint print) +void BaseError::addTrace(std::shared_ptr && e, HintFmt hint, TracePrint print) { err.traces.push_front(Trace { .pos = std::move(e), .hint = hint, .print = print }); } @@ -146,7 +146,7 @@ static bool printUnknownLocations = getEnv("_NIX_EVAL_SHOW_UNKNOWN_LOCATIONS").h * * @return true if a position was printed. */ -static bool printPosMaybe(std::ostream & oss, std::string_view indent, const std::shared_ptr & pos) { +static bool printPosMaybe(std::ostream & oss, std::string_view indent, const std::shared_ptr & pos) { bool hasPos = pos && *pos; if (hasPos) { oss << indent << ANSI_BLUE << "at " ANSI_WARNING << *pos << ANSI_NORMAL << ":"; diff --git a/src/libutil/include/nix/util/error.hh b/src/libutil/include/nix/util/error.hh index fa60d4c61..7c96112ea 100644 --- a/src/libutil/include/nix/util/error.hh +++ b/src/libutil/include/nix/util/error.hh @@ -78,7 +78,7 @@ enum struct TracePrint { }; struct Trace { - std::shared_ptr pos; + std::shared_ptr pos; HintFmt hint; TracePrint print = TracePrint::Default; }; @@ -88,7 +88,7 @@ inline std::strong_ordering operator<=>(const Trace& lhs, const Trace& rhs); struct ErrorInfo { Verbosity level; HintFmt msg; - std::shared_ptr pos; + std::shared_ptr pos; std::list traces; /** * Some messages are generated directly by expressions; notably `builtins.warn`, `abort`, `throw`. @@ -172,7 +172,7 @@ public: err.status = status; } - void atPos(std::shared_ptr pos) { + void atPos(std::shared_ptr pos) { err.pos = pos; } @@ -182,12 +182,12 @@ public: } template - void addTrace(std::shared_ptr && e, std::string_view fs, const Args & ... args) + void addTrace(std::shared_ptr && e, std::string_view fs, const Args & ... args) { addTrace(std::move(e), HintFmt(std::string(fs), args...)); } - void addTrace(std::shared_ptr && e, HintFmt hint, TracePrint print = TracePrint::Default); + void addTrace(std::shared_ptr && e, HintFmt hint, TracePrint print = TracePrint::Default); bool hasTrace() const { return !err.traces.empty(); } diff --git a/src/libutil/include/nix/util/position.hh b/src/libutil/include/nix/util/position.hh index f9c984976..90de0c0b2 100644 --- a/src/libutil/include/nix/util/position.hh +++ b/src/libutil/include/nix/util/position.hh @@ -46,12 +46,10 @@ struct Pos Pos(Pos & other) = default; Pos(const Pos & other) = default; Pos(Pos && other) = default; - Pos(const Pos * other); explicit operator bool() const { return line > 0; } - /* TODO: Why std::shared_ptr and not std::shared_ptr? */ - operator std::shared_ptr() const; + operator std::shared_ptr() const; /** * Return the contents of the source file. diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 9b4ecbda8..4dadf1550 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -166,7 +166,7 @@ Activity::Activity(Logger & logger, Verbosity lvl, ActivityType type, logger.startActivity(id, lvl, type, s, fields, parent); } -void to_json(nlohmann::json & json, std::shared_ptr pos) +void to_json(nlohmann::json & json, std::shared_ptr pos) { if (pos) { json["line"] = pos->line; diff --git a/src/libutil/position.cc b/src/libutil/position.cc index dfe0e2abb..a1d9460ed 100644 --- a/src/libutil/position.cc +++ b/src/libutil/position.cc @@ -2,19 +2,9 @@ namespace nix { -Pos::Pos(const Pos * other) +Pos::operator std::shared_ptr() const { - if (!other) { - return; - } - line = other->line; - column = other->column; - origin = other->origin; -} - -Pos::operator std::shared_ptr() const -{ - return std::make_shared(&*this); + return std::make_shared(*this); } std::optional Pos::getCodeLines() const From cdb8567473999f6728584174961e99849c347fb2 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 1 Jun 2025 20:48:49 +0000 Subject: [PATCH 2/2] libutil: Don't explicitly default special member functions Since all of the member types are copyable/movable the compiler will generate all of those by default anyway. --- src/libutil/include/nix/util/position.hh | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libutil/include/nix/util/position.hh b/src/libutil/include/nix/util/position.hh index 90de0c0b2..34cf86392 100644 --- a/src/libutil/include/nix/util/position.hh +++ b/src/libutil/include/nix/util/position.hh @@ -43,9 +43,6 @@ struct Pos Pos() { } Pos(uint32_t line, uint32_t column, Origin origin) : line(line), column(column), origin(origin) { } - Pos(Pos & other) = default; - Pos(const Pos & other) = default; - Pos(Pos && other) = default; explicit operator bool() const { return line > 0; }