refactor(treewide): modern C++ idioms #14658
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Convert the Verbosity enum from old-style `typedef enum` to modern `enum class` with cleaner value names: - lvlError -> Verbosity::Error - lvlWarn -> Verbosity::Warn - lvlNotice -> Verbosity::Notice - lvlInfo -> Verbosity::Info - lvlTalkative -> Verbosity::Talkative - lvlChatty -> Verbosity::Chatty - lvlDebug -> Verbosity::Debug - lvlVomit -> Verbosity::Vomit Added ++/-- operators with clamping behavior for safe verbosity adjustments. Added explicit casts where Verbosity is serialized to integers for protocol compatibility.
Convert the ActivityType enum from old-style `typedef enum` to modern `enum class` with cleaner value names: - actUnknown -> ActivityType::Unknown - actCopyPath -> ActivityType::CopyPath - actFileTransfer -> ActivityType::FileTransfer - actRealise -> ActivityType::Realise - actCopyPaths -> ActivityType::CopyPaths - actBuilds -> ActivityType::Builds - actBuild -> ActivityType::Build - actOptimiseStore -> ActivityType::OptimiseStore - actVerifyPaths -> ActivityType::VerifyPaths - actSubstitute -> ActivityType::Substitute - actQueryPathInfo -> ActivityType::QueryPathInfo - actPostBuildHook -> ActivityType::PostBuildHook - actBuildWaiting -> ActivityType::BuildWaiting - actFetchTree -> ActivityType::FetchTree Added Logger::Field constructor for ActivityType to support logging. Added explicit cast for protocol serialization.
Convert ResultType from typedef enum to strongly-typed enum class: - resFileLinked → ResultType::FileLinked - resBuildLogLine → ResultType::BuildLogLine - resUntrustedPath → ResultType::UntrustedPath - resCorruptedPath → ResultType::CorruptedPath - resSetPhase → ResultType::SetPhase - resProgress → ResultType::Progress - resSetExpected → ResultType::SetExpected - resPostBuildLogLine → ResultType::PostBuildLogLine - resFetchStatus → ResultType::FetchStatus Add explicit cast in daemon.cc for wire protocol serialization.
Convert SandboxMode from typedef enum to strongly-typed enum class: - smEnabled → SandboxMode::Enabled - smRelaxed → SandboxMode::Relaxed - smDisabled → SandboxMode::Disabled
Convert InternalType from typedef enum to strongly-typed enum class: - tUninitialized → InternalType::Uninitialized - tInt → InternalType::Int - tBool → InternalType::Bool - tNull → InternalType::Null - tFloat → InternalType::Float - tExternal → InternalType::External - tPrimOp → InternalType::PrimOp - tAttrs → InternalType::Attrs - tListSmall → InternalType::ListSmall - tPrimOpApp → InternalType::PrimOpApp - tApp → InternalType::App - tThunk → InternalType::Thunk - tLambda → InternalType::Lambda - tListN → InternalType::ListN - tString → InternalType::String - tPath → InternalType::Path Add explicit casts for enum arithmetic in value storage operations. Rename test cases to use descriptive names instead of enum values.
Convert HookReply from typedef enum to strongly-typed enum class: - rpAccept → HookReply::Accept - rpDecline → HookReply::Decline - rpPostpone → HookReply::Postpone
Convert Logger::Field's anonymous enum to strongly-typed enum class: - tInt → Logger::Field::Type::Int - tString → Logger::Field::Type::String Add explicit cast in daemon.cc for wire protocol serialization.
Replace iterator-based loop with structured binding range-based for when iterating over request headers map.
Replace iterator-based loop with range-based for when iterating over platform levels set.
Replace iterator-based loop with range-based for when appending saved arguments to command line.
The make_ref<T>() factory function should never have its return value ignored, as that would leak the allocated object.
Add [[nodiscard]] to hash computation and parsing functions: - newHashAllowEmpty, hashString, hashFile, compressHash - parseHashFormat, parseHashFormatOpt, printHashFormat - parseHashAlgo, parseHashAlgoOpt, printHashAlgo These are pure functions with no side effects - their only purpose is to produce a return value. Ignoring the result means the call was pointless (wasted computation) and likely indicates a logic error in the calling code.
Add [[nodiscard]] to EvalState methods where ignoring the return value indicates the call was likely pointless: - forceInt, forceFloat, forceBool: force value and return typed result - isDerivation: check if value is a derivation - isFunctor: check if value is a functor Note: forceString variants are NOT marked [[nodiscard]] because they are legitimately used for side effects (type validation) with the string accessed via Value::string_view() afterward.
Replace iterator-pair std::sort calls with std::ranges::sort for cleaner syntax where the container satisfies range concepts. Note: boost::container::small_vector doesn't satisfy range concepts, so those calls remain iterator-based.
Replace iterator-pair std::sort calls with std::ranges::sort for attribute sorting in value printing.
Mark getStr, getInt, and isNull as const member functions since they only read from the SQLite statement and don't modify any state.
Mark as const since it only queries the database state.
Mark isKnownNow() and didExist() as const since they only read state.
Mark hasData() as const in both classes since they only check buffer state without modification.
Mark isNotNARSerialisable() and typeString() as const since they only read the type member.
github-actions
bot
added
new-cli
Member
|
Some of these are unnecessarily code-churny with little benefit (especially the enum stuff, i.e. |
Replace .first/.second pair access and manual destructuring with structured bindings (auto [key, value] = ...) for improved readability. Changes across 8 files: - archive.cc: directory entry iteration - config-global.cc: settings map iteration - configuration.cc: settings and unknown settings iteration - environment-variables.cc: environment variable iteration - nar-accessor.cc: children map iteration - util.cc: rewrites map iteration - windows/processes.cc: environment variable handling - xml-writer.cc: XML attributes iteration
Replace .first/.second pair access with structured bindings for improved readability in map/pair iterations. Changes: - print.cc: attribute sorting iteration - eval.cc: static environment variable iteration - primops.cc: groupBy attribute iteration - primops/context.cc: context info iteration - primops/fromTOML.cc: TOML table iteration - json-to-value.cc: JSON attribute iteration
Replace explicit .first/.second access and separate variable declarations with structured bindings for improved readability.
Replace explicit .first/.second access with structured bindings for improved readability in attrs, cache, and fetchers.
Replace explicit .first/.second access with structured bindings for improved readability in progress-bar and common-args.
Replace explicit .first/.second access with structured bindings for improved readability in flake and lockfile handling.
Replace explicit .first/.second access with structured bindings for improved readability in nix-build, run, why-depends, flake, and user-env commands.
Member
|
I do like using I don't really think the merge conflict argument against churn holds water anymore because the LLMs will cruise through trivial merge conflicts from changes like that l. |
- Change static const to static constexpr for compile-time constants - Add constexpr to path trait functions (isPathSep, findPathSep, rfindPathSep) - Clean up redundant qualifiers (const static constexpr -> static constexpr) - Use consistent static constexpr ordering throughout
- Change const to constexpr for compile-time constants (exportMagic, maxIdsPerBuild, nixSchemaVersion, mtimeStore) - Change static const to static constexpr for class members (maxSigs) - Use consistent static constexpr ordering for MissingName
- Change const to constexpr for defaultPriority constant
- Exit: add `status = 0` default, simplify default constructor to = default - PosIdx: add `id = 0` default, simplify default constructor to = default
- Symbol: add `id = 0` default, simplify default constructor to = default - ExprAttrs: add `recursive = false` default, remove redundant initialization from constructors
- Replace chained ternary operator with or_else().value_or() in getBuildDir()
- Remove redundant intermediate checks when returning values from functions that already return std::optional (getRef, getRevCount, getLastModified)
- Replace push_back with emplace_back for Header pairs to construct directly in container - Use emplace_back for string literal arguments to avoid temporary string construction
Change isNonUriPath parameter from const std::string& to std::string_view since the function only uses the string for comparison operations.
Change filterPrintable and matchUser parameters from const std::string& to std::string_view since these functions only iterate or compare strings without storing or needing null-termination.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This is a tracking PR for my work around C++ modernization. It should not be
reviewed, changes will be fished out into individual PRs.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.