From d62cfc1c9764eb63e4fcc4c9330c78511afa276c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 19 Sep 2024 20:33:35 +0200 Subject: [PATCH] Re-introduce mkStringNoCopy (revised) In b70d22b `mkStringNoCopy()` was renamed to `mkString()`, but this is a bit risky since in code like vStringRegular.mkString("regular"); we want to be sure that the right overload is picked. (This is especially problematic since the overload that takes an `std::string_view` *does* allocate.) So let's be explicit. (Rebased from https://github.com/NixOS/nix/pull/11551) --- src/libexpr-tests/json.cc | 4 ++-- src/libexpr-tests/value/print.cc | 20 ++++++++++---------- src/libexpr/eval.cc | 14 +++++++------- src/libexpr/include/nix/expr/nixexpr.hh | 2 +- src/libexpr/include/nix/expr/symbol-table.hh | 4 ++-- src/libexpr/include/nix/expr/value.hh | 2 +- src/libexpr/primops.cc | 2 +- src/libexpr/primops/fromTOML.cc | 2 +- src/nix/nix-env/user-env.cc | 2 +- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/libexpr-tests/json.cc b/src/libexpr-tests/json.cc index c090ac5d7..8b1bd7d96 100644 --- a/src/libexpr-tests/json.cc +++ b/src/libexpr-tests/json.cc @@ -54,7 +54,7 @@ TEST_F(JSONValueTest, IntNegative) TEST_F(JSONValueTest, String) { Value v; - v.mkString("test"); + v.mkStringNoCopy("test"); ASSERT_EQ(getJSONValue(v), "\"test\""); } @@ -62,7 +62,7 @@ TEST_F(JSONValueTest, StringQuotes) { Value v; - v.mkString("test\""); + v.mkStringNoCopy("test\""); ASSERT_EQ(getJSONValue(v), "\"test\\\"\""); } diff --git a/src/libexpr-tests/value/print.cc b/src/libexpr-tests/value/print.cc index b32cba667..739d4e40b 100644 --- a/src/libexpr-tests/value/print.cc +++ b/src/libexpr-tests/value/print.cc @@ -35,14 +35,14 @@ TEST_F(ValuePrintingTests, tBool) TEST_F(ValuePrintingTests, tString) { Value vString; - vString.mkString("some-string"); + vString.mkStringNoCopy("some-string"); test(vString, "\"some-string\""); } TEST_F(ValuePrintingTests, tPath) { Value vPath; - vPath.mkString("/foo"); + vPath.mkStringNoCopy("/foo"); test(vPath, "\"/foo\""); } @@ -290,10 +290,10 @@ TEST_F(StringPrintingTests, maxLengthTruncation) TEST_F(ValuePrintingTests, attrsTypeFirst) { Value vType; - vType.mkString("puppy"); + vType.mkStringNoCopy("puppy"); Value vApple; - vApple.mkString("apple"); + vApple.mkStringNoCopy("apple"); BindingsBuilder builder(state, state.allocBindings(10)); builder.insert(state.symbols.create("type"), &vType); @@ -334,7 +334,7 @@ TEST_F(ValuePrintingTests, ansiColorsBool) TEST_F(ValuePrintingTests, ansiColorsString) { Value v; - v.mkString("puppy"); + v.mkStringNoCopy("puppy"); test(v, ANSI_MAGENTA "\"puppy\"" ANSI_NORMAL, PrintOptions{.ansiColors = true}); } @@ -342,7 +342,7 @@ TEST_F(ValuePrintingTests, ansiColorsString) TEST_F(ValuePrintingTests, ansiColorsStringElided) { Value v; - v.mkString("puppy"); + v.mkStringNoCopy("puppy"); test( v, @@ -390,7 +390,7 @@ TEST_F(ValuePrintingTests, ansiColorsAttrs) TEST_F(ValuePrintingTests, ansiColorsDerivation) { Value vDerivation; - vDerivation.mkString("derivation"); + vDerivation.mkStringNoCopy("derivation"); BindingsBuilder builder(state, state.allocBindings(10)); builder.insert(state.s.type, &vDerivation); @@ -413,7 +413,7 @@ TEST_F(ValuePrintingTests, ansiColorsError) { Value throw_ = state.getBuiltin("throw"); Value message; - message.mkString("uh oh!"); + message.mkStringNoCopy("uh oh!"); Value vError; vError.mkApp(&throw_, &message); @@ -430,12 +430,12 @@ TEST_F(ValuePrintingTests, ansiColorsDerivationError) { Value throw_ = state.getBuiltin("throw"); Value message; - message.mkString("uh oh!"); + message.mkStringNoCopy("uh oh!"); Value vError; vError.mkApp(&throw_, &message); Value vDerivation; - vDerivation.mkString("derivation"); + vDerivation.mkStringNoCopy("derivation"); BindingsBuilder builder(state, state.allocBindings(10)); builder.insert(state.s.type, &vDerivation); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 8c5646403..fd2108537 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -292,10 +292,10 @@ EvalState::EvalState( vNull.mkNull(); vTrue.mkBool(true); vFalse.mkBool(false); - vStringRegular.mkString("regular"); - vStringDirectory.mkString("directory"); - vStringSymlink.mkString("symlink"); - vStringUnknown.mkString("unknown"); + vStringRegular.mkStringNoCopy("regular"); + vStringDirectory.mkStringNoCopy("directory"); + vStringSymlink.mkStringNoCopy("symlink"); + vStringUnknown.mkStringNoCopy("unknown"); /* Construct the Nix expression search path. */ assert(lookupPath.elements.empty()); @@ -824,7 +824,7 @@ DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t) void Value::mkString(std::string_view s) { - mkString(makeImmutableString(s)); + mkStringNoCopy(makeImmutableString(s)); } static const char ** encodeContext(const NixStringContext & context) @@ -843,12 +843,12 @@ static const char ** encodeContext(const NixStringContext & context) void Value::mkString(std::string_view s, const NixStringContext & context) { - mkString(makeImmutableString(s), encodeContext(context)); + mkStringNoCopy(makeImmutableString(s), encodeContext(context)); } void Value::mkStringMove(const char * s, const NixStringContext & context) { - mkString(s, encodeContext(context)); + mkStringNoCopy(s, encodeContext(context)); } void Value::mkPath(const SourcePath & path) diff --git a/src/libexpr/include/nix/expr/nixexpr.hh b/src/libexpr/include/nix/expr/nixexpr.hh index 3c3c5e6f9..414eb5116 100644 --- a/src/libexpr/include/nix/expr/nixexpr.hh +++ b/src/libexpr/include/nix/expr/nixexpr.hh @@ -158,7 +158,7 @@ struct ExprString : Expr ExprString(std::string && s) : s(std::move(s)) { - v.mkString(this->s.data()); + v.mkStringNoCopy(this->s.data()); }; Value * maybeThunk(EvalState & state, Env & env) override; diff --git a/src/libexpr/include/nix/expr/symbol-table.hh b/src/libexpr/include/nix/expr/symbol-table.hh index ff98077ca..cb31923bf 100644 --- a/src/libexpr/include/nix/expr/symbol-table.hh +++ b/src/libexpr/include/nix/expr/symbol-table.hh @@ -113,12 +113,12 @@ public: // for multi-threaded implementations: lock store and allocator here const auto & [v, idx] = key.store.add(SymbolValue{}); if (size == 0) { - v.mkString("", nullptr); + v.mkStringNoCopy("", nullptr); } else { auto s = key.alloc.allocate(size + 1); memcpy(s, key.s.data(), size); s[size] = '\0'; - v.mkString(s, nullptr); + v.mkStringNoCopy(s, nullptr); } v.size_ = size; v.idx = idx; diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index a2833679b..9d0cf1e54 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -960,7 +960,7 @@ public: setStorage(b); } - inline void mkString(const char * s, const char ** context = 0) noexcept + void mkStringNoCopy(const char * s, const char ** context = 0) noexcept { setStorage(StringWithContext{.c_str = s, .context = context}); } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 264f3d155..515fc0626 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4349,7 +4349,7 @@ static void prim_substring(EvalState & state, const PosIdx pos, Value ** args, V if (len == 0) { state.forceValue(*args[2], pos); if (args[2]->type() == nString) { - v.mkString("", args[2]->context()); + v.mkStringNoCopy("", args[2]->context()); return; } } diff --git a/src/libexpr/primops/fromTOML.cc b/src/libexpr/primops/fromTOML.cc index 7d98a5de9..3ab594905 100644 --- a/src/libexpr/primops/fromTOML.cc +++ b/src/libexpr/primops/fromTOML.cc @@ -136,7 +136,7 @@ static void prim_fromTOML(EvalState & state, const PosIdx pos, Value ** args, Va normalizeDatetimeFormat(t); #endif auto attrs = state.buildBindings(2); - attrs.alloc("_type").mkString("timestamp"); + attrs.alloc("_type").mkStringNoCopy("timestamp"); std::ostringstream s; s << t; auto str = toView(s); diff --git a/src/nix/nix-env/user-env.cc b/src/nix/nix-env/user-env.cc index 766c6d42a..4ed93135d 100644 --- a/src/nix/nix-env/user-env.cc +++ b/src/nix/nix-env/user-env.cc @@ -56,7 +56,7 @@ bool createUserEnv( auto attrs = state.buildBindings(7 + outputs.size()); - attrs.alloc(state.s.type).mkString("derivation"); + attrs.alloc(state.s.type).mkStringNoCopy("derivation"); attrs.alloc(state.s.name).mkString(i.queryName()); auto system = i.querySystem(); if (!system.empty())