8000 10000
Skip to content

[core] Add StringRef::starts_with - #18142

Merged
bdraco merged 16 commits into
devfrom
core-stringref-starts-with
Aug 7, 2026
Merged

[core] Add StringRef::starts_with#18142
bdraco merged 16 commits into
devfrom
core-stringref-starts-with

Conversation

@bdraco
@bdraco bdraco commented Aug 7, 2026 •
edited
Loading
Copy link
Copy Markdown
Member

What does this implement/fix?

Adds a starts_with helper to StringRef, matching std::string::starts_with; a length check plus memcmp on the view. The next PR in this series stores BLE advertisement names in a fixed buffer exposed as StringRef, and the listeners that prefix match on the name (radon_eye_ble, thermopro_ble) keep working through this helper.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New developer-facing feature (adds functionality for component developers; no end-user configuration change)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — policy
  • Developer breaking change (an API change that could break external components) — policy
  • Undocumented C++ API change (removal or change of undocumented public methods that lambda users may depend on) — policy
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

  • fixes

Pull request in esphome.io with documentation (if applicable):

  • esphome/esphome.io#<esphome.io PR number goes here>

Pull request in developers.esphome.io with developer documentation (if applicable):

  • esphome/developers.esphome.io#<developers.esphome.io PR number goes here>

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040/RP2350
  • BK72xx
  • RTL87xx
  • LN882x
  • nRF52840

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@esphome esphome Bot added chained-pr This PR is chained to another PR which must be merged before this one core small-pr PR < 30 lines labels Aug 7, 2026
@bdraco bdraco mentioned this pull request Aug 7, 2026
Merged
19 tasks
@bdraco
bdraco commented Aug 7, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@codecov
codecov Bot commented Aug 7, 2026 •
edited
Loading
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.28%. Comparing base (97558ce) to head (cdf7f9d).
⚠️ Report is 2 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##              dev   #18142   +/-   ##
=======================================
  Coverage   87.28%   87.28%           
=======================================
  Files          64       64           
  Lines       14697    14697           
  Branches     2217     2217           
=======================================
  Hits        12829    12829           
  Misses       1558     1558           
  Partials      310      310
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codspeed-hq
codspeed-hq Bot commented Aug 7, 2026 •
edited
Loading
Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 153 untouched benchmarks

Comparing core-stringref-starts-with (cdf7f9d) with dev (97558ce)

Open in CodSpeed

@github-actions
github-actions Bot commented Aug 7, 2026
Copy link
Copy Markdown
Contributor

Memory Impact Analysis

Components: api
Platform: esp32-idf

Metric Target Branch This PR Change
RAM 45,648 bytes 45,648 bytes ➡️ +0 bytes (0.00%)
Flash 784,775 bytes 784,775 bytes ➡️ +0 bytes (0.00%)

Note: This analysis measures static RAM and Flash usage only (compile-time allocation).
Dynamic memory (heap) cannot be measured automatically.
⚠️ You must test this PR on a real device to measure free heap and ensure no runtime memory issues.

This analysis runs automatically when components change. Memory usage is measured from a representative test configuration.

@bdraco
bdraco commented Aug 7, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@bdraco bdraco added stacked-pr and removed chained-pr This PR is chained to another PR which must be merged before this one labels Aug 7, 2026
@bluetoothbot
bluetoothbot commented Aug 7, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot approved these changes Aug 7, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco
bdraco commented Aug 7, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@esphome esphome Bot added the has-tests label Aug 7, 2026
@bdraco
bdraco commented Aug 7, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

Base automatically changed from core-eventpool-warm to dev August 7, 2026 01:40
@bdraco
bdraco force-pushed the core-stringref-starts-with branch from 029c2e8 to 386cfd0 Compare August 7, 2026 01:40
@bluetoothbot
bluetoothbot commented Aug 7, 2026
Copy link
Copy Markdown
Contributor

PR Review — [core] Add StringRef::starts_with

Both prior suggestions are addressed and the helper is correct — merge-ready, no findings.

What's solid:

  • starts_with guards with len_ >= prefix.len_ before memcmp, so an over-long prefix can never read past the view; an empty prefix returns true, matching std::string::starts_with. It mirrors the existing compare() implementation and <cstring> is already included.
  • Using memcmp over (base_, len_) rather than strstr/strlen is the right call: it works on a view that is not null-terminated, which is exactly what the follow-up PR's fixed BLE-name buffer needs. find() right below it carries an explicit "Requires the underlying string to be null-terminated" caveat, so the new helper is not just sugar over find(...) == 0.
  • The const char * overload is what keeps the named consumers compiling untouched: esphome/components/radon_eye_ble/radon_eye_listener.cpp:13 and esphome/components/thermopro_ble/thermopro_ble.cpp:106 today call starts_with(...) on the const std::string & returned by ble_device_base::BLEDevice::get_name(). Verified in-tree — the PR's stated rationale holds.
  • No overload ambiguity: because StringRef(const char *) and StringRef(const std::string &) are both explicit, a string literal binds to the const char * overload by standard conversion and never competes with the other two.

Prior findings, both now resolved:

  • tests/components/core/test_string_ref.cpp covers the cases that mattered — empty prefix on a non-empty and an empty view, prefix longer than the view, a deliberately non-null-terminated buffer, and a bounded StringRef prefix proving bytes past the prefix length are not compared. Namespace esphome::core::testing matches the sibling files, and script/build_helpers.py globs tests/components/<component>/*.cpp, so no registration change is needed. Lengths in the fixtures are correct ("FR:R20:12345", 12, "TP96", 4).

  • The const std::string & overload is present, so the API is now symmetric with compare()'s three forms.

  • No blocking or important issues found

  • Diff matches the PR description; no scope creep

Checklist

  • Bounds safety — no read past the view
  • Edge cases match std::string::starts_with semantics (empty prefix, over-long prefix)
  • Required headers present ( for memcmp)
  • No overload ambiguity introduced (both competing ctors are explicit)
  • API consistent with sibling methods (StringRef / const char * / const std::string &)
  • New code branches covered by host tests
  • Test file discoverable without registration change (glob in script/build_helpers.py)
  • Test namespace matches sibling files in tests/components/core
  • No backward-incompatible change (pure addition)
  • Diff matches PR description, no scope creep
  • Embedded constraints respected (no heap allocation, fully inlined)

Automated review by Kōan (Claude) HEAD=cdf7f9d 1 min 50s

bluetoothbot
bluetoothbot approved these chan 9E88 ges Aug 7, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco
bdraco marked this pull request as ready for review August 7, 2026 01:46
@bdraco
bdraco requested a review from a team as a code owner August 7, 2026 01:46
Copilot AI lite review requested due to automatic review settings August 7, 2026 01:46
@esphome
esphome Bot commented Aug 7, 2026
Copy link
Copy Markdown
Contributor

👋 Hi there! This PR modifies 1 file(s) with codeowners.

@esphome/core - As codeowner(s) of the affected files, your review would be appreciated! 🙏

Note: Automatic review request may have failed, but you're still welcome to review.

Copilot AI reviewed Aug 7, 2026
View reviewed changes
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends ESPHome’s core StringRef utility with a starts_with() helper (similar to std::string::starts_with) to support prefix-matching on bounded, potentially non-null-terminated buffers—needed for upcoming BLE advertisement name handling that will expose names as StringRef.

Changes:

  • Added StringRef::starts_with() overloads for StringRef, const char*, and std::string.
  • Added new GoogleTest coverage for prefix-matching behavior, including non-terminated buffer cases and overload behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
esphome/core/string_ref.h Adds starts_with() implementation and overloads on the StringRef view type.
tests/components/core/test_string_ref.cpp Introduces unit tests validating correct prefix matching across edge cases and overloads.

Comment thread esphome/core/string_ref.h
@bdraco
bdraco commented Aug 7, 2026
Copy link
Copy Markdown
Member Author

thanks

@bdraco
bdraco merged commit e58ba59 into dev Aug 7, 2026
67 checks passed
@bdraco
bdraco deleted the core-stringref-starts-with branch August 7, 2026 01:58
@bdraco bdraco mentioned this pull request Aug 7, 2026
Merged
19 tasks
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@swoboda1337 swoboda1337 swoboda1337 approved these changes
+1 more reviewer
@bluetoothbot bluetoothbot bluetoothbot approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core has-tests small-pr PR < 30 lines stacked-pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bdraco @bluetoothbot @swoboda1337
0