Skip to content

chore: remove unnecessary String clones in fmq functions#8

Merged
nhomble merged 1 commit intomainfrom
chore/remove-unnecessary-clones
Dec 30, 2025
Merged

chore: remove unnecessary String clones in fmq functions#8
nhomble merged 1 commit intomainfrom
chore/remove-unnecessary-clones

Conversation

@nhomble
Copy link
Owner

@nhomble nhomble commented Dec 30, 2025

Use &str references instead of cloning doc.frontmatter since run() only needs to borrow the string, not own it.

Use &str references instead of cloning doc.frontmatter since run()
only needs to borrow the string, not own it.
@claude
Copy link

claude bot commented Dec 30, 2025

Code Review: Remove unnecessary String clones

Summary

This is an excellent optimization that eliminates unnecessary allocations by using string references instead of clones. The changes are clean, correct, and improve performance with zero downside.

✅ Code Quality & Best Practices

Excellent work! The changes demonstrate:

  • Zero-cost abstraction awareness: Using &str borrows instead of cloning String values
  • Consistent patterns: Both fmq() and fmq_reader() now follow the same pattern
  • Explicit type annotations: Adding let frontmatter: &str = ... makes the code more readable and intentional

The code properly leverages Rust's ownership system - since run() only needs to borrow the frontmatter string (not own it), references are the right choice.

🐛 Potential Issues

None identified. The changes are safe because:

  • run() at src/query.rs:55 accepts &str, so passing references is valid
  • The lifetime of doc.frontmatter extends through the entire function, so the reference remains valid
  • The empty frontmatter case correctly uses a string literal "{}" instead of the previous allocation

⚡ Performance Considerations

Positive impact:

  • Eliminates 2 allocations per call (one in each function)
  • Removes unnecessary clones - the previous .clone() created a heap allocation that was immediately discarded
  • For the empty frontmatter case: changed from "{}".to_string() (heap allocation) to "{}" (stack-only string literal)

This is exactly the kind of optimization Clippy would suggest with clippy::unnecessary_clone.

🔒 Security Concerns

None. String borrows don't introduce any security risks.

🧪 Test Coverage

The existing comprehensive test suite in tests/integration.rs covers:

  • Query operations (tests/fixtures/queries)
  • Mutation operations (tests/fixtures/mutations)
  • Error handling (tests/fixtures/errors)
  • Init mode (tests/fixtures/init)

Since this is a pure refactoring that doesn't change behavior, the existing tests are sufficient. Running the test suite would verify correctness.

💡 Additional Observations

I noticed one remaining .clone() at src/lib.rs:81 in the format_output() function:

serde_json::Value::String(s) => s.clone(),

This one appears necessary since we're extracting the owned String from the Value, and the caller needs ownership. However, if format_output() returned &str or Cow<str> in the future, this could potentially be optimized too - though that would require API changes and may not be worth it.

Recommendation

✅ Approve and merge. This is a clean, beneficial change with no downsides.

Nice work on identifying and fixing this inefficiency! 🎯

@nhomble nhomble merged commit 7902b54 into main Dec 30, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant