Fix wrong checks for some integer config records #12643

moonchen · 2025-11-05T22:22:38Z

RECC_INT expects a numeric range, and not a regex. Regex should be used with RECC_STR.

Fix configs where RECC_INT is used with an invalid range.
Add a static_assert so that RECC_INT is always used with a range.
Make RECC_INT range check more robust.
Add unit tests for range check.

RECC_INT expects a numeric range, and not a regex. Regex should be used with RECC_STR. 1. Fix configs where RECC_INT is used with an invalid range. 2. Add a static_assert so that RECC_INT is always used with a range. 3. Make RECC_INT range check more robust. 4. Add unit tests for range check.

Copilot

Pull Request Overview

This PR refactors the recordRangeCheck function to use modern C++ parsing (std::from_chars) instead of atoi, which provides stricter validation and better error handling. The key changes include:

Replaced atoi with std::from_chars for strict integer parsing
Added compile-time validation to ensure RECC_INT patterns follow the [lower-upper] format
Corrected several RECC_INT patterns in RecordsConfig.cc to use proper range syntax [0-1] instead of regex patterns like ^[0-9]+$ or [01]
Added comprehensive unit tests for the refactored range checking functionality

Reviewed Changes

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

File	Description
src/records/RecUtils.cc	Refactored `recordRangeCheck` to use `std::from_chars` for strict parsing and added `parse_int` helper function
src/records/RecordsConfig.cc	Corrected RECC_INT patterns to use range format `[lower-upper]`, changed some to RECC_STR for regex patterns, and added compile-time validation for RECC_INT patterns
src/records/unit_tests/test_RecUtils.cc	Added comprehensive unit tests covering valid ranges, boundaries, error cases, and edge cases
src/records/CMakeLists.txt	Added new test file to the test executable

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-05T22:24:21Z

src/records/RecUtils.cc

+  // Find the dash separator
+  auto dash_pos = range.find('-');
+  if (dash_pos == std::string_view::npos) {
+    return false; // No dash found
+  }


The implementation will incorrectly find the first dash in patterns with negative numbers like [-10-0], resulting in parsing an empty string before the dash as the lower bound. While the test correctly validates that negative ranges should be rejected, this bug could cause silent failures or unexpected behavior. Consider adding a comment explaining that negative ranges are not supported, or add an explicit check to reject patterns starting with - after the opening bracket.

Suggested change

// Find the dash separator

auto dash_pos = range.find('-');

if (dash_pos == std::string_view::npos) {

return false; // No dash found

}

// Negative ranges are not supported. Explicitly reject if lower or upper bound starts with '-'.

auto dash_pos = range.find('-');

if (dash_pos == std::string_view::npos) {

return false; // No dash found

}

if (range.size() > 0 && range[0] == '-') {

return false; // Lower bound is negative, not supported

}

if (dash_pos + 1 < range.size() && range[dash_pos + 1] == '-') {

return false; // Upper bound is negative, not supported

}

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T15:13:41Z

src/records/RecUtils.cc

+  if (start != 0) {
+    Warning("recordRangeCheck: pattern '%s' does not start with '['", pattern);
+    return false; // No '[' found


The error message on line 401 says 'does not start with' but the actual check allows '[' to appear at any position (or not at all) via find('['). If start != 0, it means '[' was found but not at position 0. If '[' is not found at all, find returns std::string_view::npos. The message should distinguish between these cases: 'pattern does not start with' vs 'pattern does not contain'. Consider checking for npos explicitly.

Suggested change

if (start != 0) {

Warning("recordRangeCheck: pattern '%s' does not start with '['", pattern);

return false; // No '[' found

if (start == std::string_view::npos) {

Warning("recordRangeCheck: pattern '%s' does not contain '['", pattern);

return false;

} else if (start != 0) {

Warning("recordRangeCheck: pattern '%s' does not start with '['", pattern);

return false;

Starting with a [ is a stronger requirement than containing a [. We should tell the dev the stronger requirement up front to avoid multiple iterations when fixing the range syntax.

Copilot · 2025-11-06T15:13:41Z

src/records/RecordsConfig.cc

+// Parse an integer (possibly negative) at compile time
+// Updates index i to point past the parsed number
+// TODO: C++23 has std::from_chars for constexpr integer parsing


The parse_int function lacks documentation explaining its purpose and behavior. Add a docstring explaining that this function parses an integer (possibly negative) at compile time, updates index i to point past the parsed number, and returns true on success. This is especially important given the TODO comment on line 1585 about C++23.

Suggested change

// Parse an integer (possibly negative) at compile time

// Updates index i to point past the parsed number

// TODO: C++23 has std::from_chars for constexpr integer parsing

/**

* Parses an integer (possibly negative) from the given string view at compile time.

* Updates the index `i` to point past the parsed number.

* Returns true if a valid integer was parsed, false otherwise.

*

* This function is intended for compile-time validation of integer patterns.

* Note: C++23 introduces std::from_chars for constexpr integer parsing.

*

* @param s The string view to parse.

* @param i The index into `s` where parsing starts; updated to point past the parsed integer.

* @return true if parsing succeeded, false otherwise.

*/

Sounds good, but use @param[in] and @param[out]. The AI's don't seem to know about the [in]/[out] qualifiers.

You resolved this, but I don't see the use of @param[in] and @param[out].

src/records/RecordsConfig.cc

bneradt

Looks great. The more help we can give to the dev in updating these fields the better.

bneradt · 2025-11-06T16:14:40Z

src/records/RecordsConfig.cc

+// Parse an integer (possibly negative) at compile time
+// Updates index i to point past the parsed number
+// TODO: C++23 has std::from_chars for constexpr integer parsing


Sounds good, but use @param[in] and @param[out]. The AI's don't seem to know about the [in]/[out] qualifiers.

Co-authored-by: Copilot <[email protected]>

Updated parameter documentation to use Doxygen style.

brbzull0 · 2025-11-10T10:53:11Z

[approve ci]

moonchen · 2025-11-10T14:19:24Z

@bneradt I've added the param[in] and out comments.

moonchen requested a review from Copilot November 5, 2025 22:22

Copilot AI reviewed Nov 5, 2025

View reviewed changes

moonchen added 2 commits November 5, 2025 17:50

Add support for negative ranges

681ddb3

Dodge printf portability issue

220a2fe

moonchen requested a review from Copilot November 6, 2025 15:11

Copilot AI reviewed Nov 6, 2025

View reviewed changes

bneradt requested changes Nov 6, 2025

View reviewed changes

moonchen and others added 3 commits November 6, 2025 15:10

Apply suggestions from code review

3fc1bb6

Co-authored-by: Copilot <[email protected]>

Merge branch 'master' into fix-rec-check-types

1fcab70

Refine parameter documentation in parse_int function

6a2c978

Updated parameter documentation to use Doxygen style.

brbzull0 added the Records Records related code. label Nov 10, 2025

brbzull0 assigned moonchen Nov 10, 2025

moonchen requested a review from bneradt November 10, 2025 14:18

apache deleted a comment from moonchen Nov 10, 2025

-// Parse an integer (possibly negative) at compile time
-// Updates index i to point past the parsed number
-// TODO: C++23 has std::from_chars for constexpr integer parsing
+/**
+ * Parses an integer (possibly negative) from the given string view at compile time.
+ * Updates the index `i` to point past the parsed number.
+ * Returns true if a valid integer was parsed, false otherwise.
+ *
+ * This function is intended for compile-time validation of integer patterns.
+ * Note: C++23 introduces std::from_chars for constexpr integer parsing.
+ *
+ * @param s The string view to parse.
+ * @param i The index into `s` where parsing starts; updated to point past the parsed integer.
+ * @return true if parsing succeeded, false otherwise.
+ */

Fix wrong checks for some integer config records #12643

Are you sure you want to change the base?

Fix wrong checks for some integer config records #12643

Conversation

moonchen commented Nov 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

moonchen Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

bneradt Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bneradt Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bneradt left a comment

Choose a reason for hiding this comment

Uh oh!

bneradt Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brbzull0 commented Nov 10, 2025

Uh oh!

moonchen commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bneradt Nov 6, 2025 •

edited

Loading

bneradt Nov 6, 2025 •

edited

Loading