From 39d8de32e7dadee2605551d8a2da2f336563af9d Mon Sep 17 00:00:00 2001 From: helly25 <6420169+helly25@users.noreply.github.com> Date: Sat, 27 Jun 2026 15:58:30 +0100 Subject: [PATCH 1/2] engine: mode-scoped traversal defaults (modern parallel+sorted, find unordered) Completes the deferred piece of the parallel walk (#43/#54). RunFind gains an optional registry::Style: when the user gives no --sort / -j, the default is mode-scoped - kXff (modern) sorts each directory (--sort=dir) and runs a capped parallel walk (max(1, min(cores-1, 15))); kFind matches find (unordered) but saturates cores. std::nullopt keeps the conservative unordered + sequential default, so the ~32 in-process RunFind callers and conformance are untouched; the CLI passes the active style (config::ActiveStyle / argv[0] dispatch). run_test::ModeScopedSortDefault locks the behavior (kXff deterministic dir- sorted; kFind same set, unordered). Full suite green under default and asan+ubsan; tsan unaffected (test callers stay sequential; walk_test already covers the parallel path). --- xff/cli/main.cc | 3 ++- xff/engine/run.cc | 46 +++++++++++++++++++++++++++++++----------- xff/engine/run.h | 16 ++++++++++++++- xff/engine/run_test.cc | 34 +++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/xff/cli/main.cc b/xff/cli/main.cc index 5f7634c..fdea9a0 100644 --- a/xff/cli/main.cc +++ b/xff/cli/main.cc @@ -163,6 +163,7 @@ int main(int argc, char** argv) { [](std::string_view record) { std::cout.write(record.data(), static_cast(record.size())); }, [](std::string_view path, absl::Status status) { std::cerr << "xff: " << path << ": " << status.message() << "\n"; - }); + }, + style); // mode-scoped traversal defaults (modern -> sorted + parallel; find -> unordered) return errors == 0 ? 0 : 2; } diff --git a/xff/engine/run.cc b/xff/engine/run.cc index 1b2ac0c..43d4039 100644 --- a/xff/engine/run.cc +++ b/xff/engine/run.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -108,13 +109,30 @@ SymlinkMode ResolveSymlinkMode(const std::vector& globals) { return mode; } +// The mode-scoped default worker count when `-j` is absent (docs/design-parallel.md +// "Parallelism control"): modern (kXff) leaves a core for the consumer and caps at +// 15 to avoid oversubscription; find/fd/rg saturate cores; an unset style stays +// sequential (the conservative in-process / test default). +std::size_t DefaultWorkers(std::optional style) { + if (!style.has_value()) { + return 1; + } + const unsigned detected = std::thread::hardware_concurrency(); + const std::size_t cores = detected == 0 ? 1 : detected; + if (*style == registry::Style::kXff) { + return std::max(1, std::min(cores - 1, 15)); + } + return cores; +} + // xff --sort=none|dir|subtree|tree: per-directory sibling ordering for the walk // (see docs/design-parallel.md). `none` keeps readdir order (find's default); // `dir` sorts each directory's listing; `subtree` adds contiguous subtrees; -// `tree` is a total path order. Bare --sort and the legacy `name` mean `dir`. +// `tree` is a total path order. Bare --sort and the legacy `name` mean `dir`. The +// default is mode-scoped: modern (kXff) sorts each directory, find stays unordered. // Leading global, last occurrence wins. -SortOrder ResolveSort(const std::vector& globals) { - SortOrder sort = SortOrder::kNone; +SortOrder ResolveSort(const std::vector& globals, std::optional style) { + SortOrder sort = style == registry::Style::kXff ? SortOrder::kDir : SortOrder::kNone; for (const std::string& global : globals) { if (global == "--sort" || global == "--sort=dir" || global == "--sort=name") { sort = SortOrder::kDir; @@ -130,12 +148,11 @@ SortOrder ResolveSort(const std::vector& globals) { } // xff -jN / --jobs=N: worker threads for the parallel directory read-ahead (see -// docs/design-parallel.md). `1` (the default) is the sequential walk. Leading -// global, last valid occurrence wins; a non-positive or unparseable value is -// ignored. Mode-scoped auto-defaults (e.g. modern -> all cores) arrive with the -// mode mechanism (#54); until then parallelism is explicit. -std::size_t ResolveJobs(const std::vector& globals) { - std::size_t jobs = 1; +// docs/design-parallel.md). When absent, the count is mode-scoped (DefaultWorkers). +// Leading global, last valid occurrence wins; a non-positive or unparseable value +// is ignored. +std::size_t ResolveJobs(const std::vector& globals, std::optional style) { + std::size_t jobs = DefaultWorkers(style); for (const std::string& global : globals) { std::string_view value; if (global.starts_with("--jobs=")) { @@ -455,7 +472,12 @@ std::optional UnusedCaptureName(const parser::Expr& expr, const std } // namespace -int RunFind(const parser::Command& command, const vfs::FileSystem& fs, EmitFn emit, WalkErrorFn on_error) { +int RunFind( + const parser::Command& command, + const vfs::FileSystem& fs, + EmitFn emit, + WalkErrorFn on_error, + std::optional style) { const parser::Expr* const expression = command.expression.get(); const bool has_action = expression != nullptr && ContainsAction(*expression); // --implicit-print=yes|no overrides find's default-print rule (otherwise !has_action). @@ -476,8 +498,8 @@ int RunFind(const parser::Command& command, const vfs::FileSystem& fs, EmitFn em } WalkOptions options; options.symlinks = ResolveSymlinkMode(command.globals); - options.sort = ResolveSort(command.globals); - options.workers = ResolveJobs(command.globals); + options.sort = ResolveSort(command.globals, style); + options.workers = ResolveJobs(command.globals, style); const render::Format format = ResolveFormat(command.globals); const std::optional tmpl = ResolveTemplate(command.globals); // A -capture whose {capture.NAME} is never referenced ran a subprocess for diff --git a/xff/engine/run.h b/xff/engine/run.h index a7004ee..4cd24c6 100644 --- a/xff/engine/run.h +++ b/xff/engine/run.h @@ -16,9 +16,12 @@ #ifndef XFF_ENGINE_RUN_H_ #define XFF_ENGINE_RUN_H_ +#include + #include "xff/engine/evaluate.h" #include "xff/engine/walk.h" #include "xff/parser/ast.h" +#include "xff/registry/descriptor.h" #include "xff/vfs/filesystem.h" namespace xff::engine { @@ -30,7 +33,18 @@ namespace xff::engine { // // Returns the number of per-path errors encountered (0 == clean). The CLI maps // a nonzero count to exit 2; the full exit-code model is a follow-up. -int RunFind(const parser::Command& command, const vfs::FileSystem& fs, EmitFn emit, WalkErrorFn on_error); +// +// `style` selects the mode-scoped traversal defaults applied when the user gives +// no `--sort` / `-j`: kXff (modern) sorts each directory (`--sort=dir`) and runs +// a capped parallel walk; kFind matches find (unordered) but saturates cores. +// `std::nullopt` keeps the conservative defaults (unordered, single-threaded) and +// is what the in-process callers/tests use; the CLI passes the active style. +int RunFind( + const parser::Command& command, + const vfs::FileSystem& fs, + EmitFn emit, + WalkErrorFn on_error, + std::optional style = std::nullopt); } // namespace xff::engine diff --git a/xff/engine/run_test.cc b/xff/engine/run_test.cc index f6c29cf..a2fa901 100644 --- a/xff/engine/run_test.cc +++ b/xff/engine/run_test.cc @@ -109,6 +109,28 @@ struct RunTest : ::testing::Test { return records; } + // Runs the bare root under `style` to exercise the mode-scoped traversal + // defaults (RunFind's `style`), returning records with the terminator stripped. + std::vector RunStyled(registry::Style style) { + const auto command = parser::Parse({root_.string()}); + EXPECT_THAT(command, IsOk()); + std::vector records; + if (!command.ok()) { + return records; + } + RunFind( + *command, fs_, + [&](std::string_view record) { + std::string text(record); + if (!text.empty() && (text.back() == '\n' || text.back() == '\0')) { + text.pop_back(); + } + records.push_back(std::move(text)); + }, + [](std::string_view, absl::Status) {}, style); + return records; + } + vfs::LocalFs fs_; fs::path root_; int last_errors_ = 0; @@ -120,6 +142,18 @@ TEST_F(RunTest, NoExpressionPrintsEverything) { EXPECT_THAT(last_errors_, 0); } +TEST_F(RunTest, ModeScopedSortDefault) { + // With no --sort, the active style picks the default: modern (kXff) sorts each + // directory's listing, so the walk is deterministic (root, then a.txt < b.md < + // sub as a block, then sub's contents). find leaves it unordered (same set). + EXPECT_THAT( + RunStyled(registry::Style::kXff), + ::testing::ElementsAre(root_.string(), Path("a.txt"), Path("b.md"), Path("sub"), Path("sub/c.txt"))); + EXPECT_THAT( + RunStyled(registry::Style::kFind), + UnorderedElementsAre(root_.string(), Path("a.txt"), Path("b.md"), Path("sub"), Path("sub/c.txt"))); +} + TEST_F(RunTest, SortNameVisitsSiblingsInDeterministicOrder) { // --sort=name orders each directory's entries by name, so the whole walk is // deterministic: root first, then a.txt < b.md < sub, then sub/c.txt. ElementsAre From 9474b6295be5894d3132b537572fdd4d00617513 Mon Sep 17 00:00:00 2001 From: helly25 <6420169+helly25@users.noreply.github.com> Date: Sat, 27 Jun 2026 16:03:56 +0100 Subject: [PATCH 2/2] test+style: use unqualified ElementsAre; document the no-::testing-prefix rule run_test's ModeScopedSortDefault wrote ::testing::ElementsAre inline even though the file already has `using ::testing::ElementsAre;` - use the bare name. STYLE_CPP.md now states the rule explicitly: bring matchers in with a using and never write the ::testing:: prefix inside an EXPECT_THAT/ASSERT_THAT expression (fixture utilities like ::testing::Test are exempt). Audited the suite: this was the only inline-qualified matcher. --- STYLE_CPP.md | 5 +++++ xff/engine/run_test.cc | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/STYLE_CPP.md b/STYLE_CPP.md index b99978c..cbfa1df 100644 --- a/STYLE_CPP.md +++ b/STYLE_CPP.md @@ -300,6 +300,11 @@ substitute for a committed test. Tests use GoogleTest + GoogleMock with these co give far better failure messages. The accepted exception is the boolean `EXPECT_TRUE` / `EXPECT_FALSE` (and `ASSERT_TRUE` / `ASSERT_FALSE`), which read fine on their own. Within a single test keep one style - do not mix, say, `EXPECT_TRUE(x)` and `EXPECT_THAT(y, IsTrue())`. +- **Name matchers unqualified - never the `::testing::` prefix inline.** Bring each matcher in with + a `using ::testing::Foo;` (or `using ::mbo::testing::Foo;`) in the test file's anonymous namespace + and use the bare name in the `EXPECT_THAT` / `ASSERT_THAT` expression; a `::testing::Foo(...)` + written inline in an assertion is the smell to fix by adding the `using`. (Fixture utilities such + as `::testing::Test` / `::testing::TempDir` are not matchers and keep their qualification.) - **`Eq` is optional - a readability choice, not a rule.** `EXPECT_THAT(x, value)` auto-wraps a bare value in `Eq`, so both forms compile. The value of `EXPECT_THAT` is that the line reads as a sentence - `EXPECT_THAT(foo, Eq(25))` is "expect that foo equals 25" - so keep `Eq` where it diff --git a/xff/engine/run_test.cc b/xff/engine/run_test.cc index a2fa901..7664462 100644 --- a/xff/engine/run_test.cc +++ b/xff/engine/run_test.cc @@ -148,7 +148,7 @@ TEST_F(RunTest, ModeScopedSortDefault) { // sub as a block, then sub's contents). find leaves it unordered (same set). EXPECT_THAT( RunStyled(registry::Style::kXff), - ::testing::ElementsAre(root_.string(), Path("a.txt"), Path("b.md"), Path("sub"), Path("sub/c.txt"))); + ElementsAre(root_.string(), Path("a.txt"), Path("b.md"), Path("sub"), Path("sub/c.txt"))); EXPECT_THAT( RunStyled(registry::Style::kFind), UnorderedElementsAre(root_.string(), Path("a.txt"), Path("b.md"), Path("sub"), Path("sub/c.txt")));