refactor: Refactor CompileParseRules.cc for type safety, modern C++, and documentation by grahamsedman · Pull Request #13196 · apache/trafficserver

grahamsedman · 2026-05-23T21:19:53Z

📌 Summary

This PR refactors src/tscore/CompileParseRules.cc to address compilation errors, type safety issues, modern C++ best practices, and documentation gaps. The changes ensure compatibility across 32-bit and 64-bit targets, fix narrowing conversion warnings, and improve code clarity and maintainability.

✅ Problems Addressed

Problem	Root Cause	Solution
Narrowing conversion errors	`char` is signed on x86, causing negative values (e.g., `-128`) for ASCII >127, which cannot be narrowed to `unsigned char` on 32-bit targets.	Use `uint8_t` for character arrays and explicitly cast to `unsigned` when writing to files.
Unused header	`#include "tscore/ink_string.h"` is included but never used.	Removed the unused header.
Poor header organization	Headers were split into two sections, reducing readability.	Consolidated all headers at the top of the file.
Missing documentation	No Doxygen comments for the file, functions, or variables.	Added comprehensive Doxygen documentation.
Non-thread-safe `uint_to_binary`	The function used a `static char buf[33]`, making it unsafe for concurrent use.	Refactored to return `std::string` (RAII-managed, thread-safe).
C-style I/O	Used `FILE*` and `fprintf`, which are less type-safe and require manual resource management.	Replaced with `std::ofstream` (RAII-based, type-safe).
Poor Doxygen practice	`@section license` causes warnings due to duplicate labels across files.	Removed `@section` and kept the license text directly in the file header.
Unused macro	`#define COMPILE_PARSE_RULES` is defined but never used.	Removed the macro.
Outdated data types	Used `unsigned int` and `char` instead of fixed-width types (`uint8_t`, `uint32_t`).	Replaced with `<cstdint>` types for portability and clarity.

🔧 Key Changes

1. File Header & Includes

Removed: #define COMPILE_PARSE_RULES, #include "tscore/ink_string.h".
Added: <cstdint>, <fstream>, <iomanip>, <string>.
Refactored: Consolidated all includes at the top of the file.
Documentation: Added a detailed Doxygen file header explaining the purpose, generated files, and modern C++ features used.

2. Data Types

Bitmask arrays: Changed from unsigned int to uint32_t for explicit size and future-proofing.
Character arrays: Changed from char to uint8_t to guarantee an unsigned range (0-255) and avoid sign-extension issues.
Loop variables: Changed from int to uint16_t or uint8_t for semantic clarity.

3. `uint_to_binary` Function

Before: Used a static char buf[33] (non-thread-safe).
After: Returns std::string (thread-safe, RAII-managed).
Parameter: Changed from unsigned int to uint32_t for explicit size.

4. File I/O

Before: Used FILE* and fprintf (C-style, manual resource management).
After: Uses std::ofstream (RAII-based, type-safe, modern C++).
Format specifiers: Replaced %d with static_cast<unsigned> to avoid sign-extension issues when printing uint8_t values.

5. Doxygen Documentation

Added detailed comments for:
- File purpose and generated outputs.
- Each global array (parseRulesCType, tparseRulesCType, etc.).
- The uint_to_binary function.
- The main function (including its steps and classification functions).

🧪 Testing

Compilation: Verified on x86 (signed char) and ARM (unsigned char) targets.
Output: Confirmed that generated files (ParseRulesCType, ParseRulesCTypeToUpper, ParseRulesCTypeToLower) contain correct values (no negative numbers for ASCII >127).
Thread Safety: The refactored uint_to_binary is now thread-safe.
Standards: Meets C++ 20 / C++ 23 and Mozilla Mozilla style guide.

- Replace C-style file I/O (fopen, fprintf, fclose) with std::ofstream for RAII-based file handling - Use fixed-width integer types (uint32_t, uint8_t) instead of unsigned int and char for portability and clarity - Refactor uint_to_binary function to use std::string instead of static buffer for thread safety - Add comprehensive Doxygen documentation for the file, functions, and arrays - Remove @section license License from file header to prevent Doxygen warnings about multiple use of section label, as @section is intended for major structured documentation sections, not repetitive boilerplate - Remove obsolete COMPILE_PARSE_RULES macro and ink_string.h dependency - Improve output formatting using std::setw, std::setfill, and std::hex for consistent alignment - Replace int loop variables with uint16_t for better type safety - Add static_cast for explicit type conversions

- Replace `1` with `1U` in bit shift operation to prevent undefined behaviour when shifting into sign bit of signed integer - Shifting a signed integer (e.g., `1 << 31`) into its sign bit invokes undefined behaviour per the C++ standard - Using an unsigned literal (`1U`) ensures well-defined behaviour for all shift amounts

…d behaviour - Change `char cc` to `unsigned char cc` to ensure consistent handling of byte values (0-255) - `char` can be signed or unsigned depending on the platform, causing implementation-defined behaviour for values > 127 - Using `unsigned char` guarantees correct interpretation of all byte values

- Rename functions to PascalCase (uint_to_binary → UintToBinary) - Add `g` prefix to global variables (example: parseRulesCType → gParseRulesCType) - Rename ParseRules classification functions and constants to PascalCase (is_* → Is*, *_BIT → IS_*) - Add file existence checks before writing with error messages to stderr - Add `#include <iostream>` for std::cerr - Rename `fp` to `outputFile` for improved clarity - Change `char` to `unsigned char` for byte value handling - Update all Doxygen comments to reflect new naming conventions

- Replace `index` with `i` to match loop variable - Replace `currentChar` with `cc` to match variable declaration - Use PascalCase for ParseRules functions (ink_tolower → InkTolower, ink_toupper → InkToupper) - Use PascalCase for UintToBinary function - Replace `fp` with `outputFile` in last file writing block for consistency

- Split file header into separate C-style comment block for license and Doxygen block for documentation - Required to pass Apache Release Audit Tool (Rat) - Fix formatting: indent Apache license URL for consistency - Fix formatting: remove extra space before "Fixed-width integer types"

grahamsedman · 2026-05-25T13:08:28Z

Hi,

I noticed this PR is failing the Jenkins AuTest stages. I investigated the failure, and it is not related to my code changes.

The tests/prepare_proxy_verifier.sh script has a hardcoded SHA1 hash on line 44:
expected_sha1="e11b5867a56c5ffd496b18c901f1273e9c120a47"

The tarball hosted at https://ci.trafficserver.apache.org/bintray/proxy-verifier-v3.1.2.tar.gz has apparently been repackaged. I downloaded the tarball locally, and the new correct SHA1 hash is:**
0a60c646cbc9326abb2fbc397cb9efa8c08a807a

Could a maintainer please update line 44 in prepare_proxy_verifier.sh on the master branch with this new hash? Once that is fixed, I believe this PR will pass CI.

Thank you!

Incorporate latest upstream changes from master to resolve potential merge conflicts and ensure feature branch stays current with the project's master branch.

masaori335 · 2026-05-29T04:50:09Z

[approve ci]

Copilot

Pull request overview

This PR refactors the src/tscore/CompileParseRules.cc build-time generator that produces the ParseRulesCType* lookup table include files used by src/tscore/ParseRules.cc, aiming to improve type-safety and modernize implementation (C++ I/O, fixed-width types, thread-safety).

Changes:

Reworked table generation logic and types (e.g., uint32_t / uint8_t) and updated the binary formatting helper to return std::string.
Switched file output from FILE* / fprintf to std::ofstream with iostream formatting.
Added extensive Doxygen documentation and reorganized includes.

+  for (uint16_t i = 0; i < 256; i++) {
+    gTparseRulesCType[i]        = 0;
+    gTparseRulesCTypeToLower[i] = static_cast<uint8_t>(ParseRules::InkTolower(i));
+    gTparseRulesCTypeToUpper[i] = static_cast<uint8_t>(ParseRules::InkToupper(i));

-    if (ParseRules::is_char(c)) {
-      tparseRulesCType[c] |= is_char_BIT;
+    if (ParseRules::IsChar(i)) {
+      gTparseRulesCType[i] |= IS_CHAR_BIT;
    }


+    unsigned char cc = static_cast<unsigned char>(i);
+
+    if (ParseRules::IsPchar(&cc)) {
+      gTparseRulesCType[i] |= IS_PCHAR_BIT;
    }


+  for (uint16_t i = 0; i < 256; ++i) {
+    outputFile << "/* " << std::setw(3) << i << " (" << (isprint(i) ? static_cast<char>(i) : '?') << ") */\t";
+    outputFile << "0x" << std::hex << std::setw(8) << std::setfill('0') << gTparseRulesCType[i] << (i != 255 ? ",\t\t" : "\t\t");
+    outputFile << "/* [" << UintToBinary(gTparseRulesCType[i]) << "] */\n";


+#include <cstdio>
+#include <cctype>


+#include <string>

-      http://www.apache.org/licenses/LICENSE-2.0
+#include "tscore/ParseRules.h"


+      return 1;
+    }
+    for (uint16_t i = 0; i < 256; ++i) {
+      outputFile << "(uint8_t)" << static_cast<unsigned>(gTparseRulesCTypeToUpper[i]) << (i != 255 ? ',' : ' ') << '\n';


+      return 1;
+    }
+    for (uint16_t i = 0; i < 256; ++i) {
+      outputFile << "(uint8_t)" << static_cast<unsigned>(gTparseRulesCTypeToLower[i]) << (i != 255 ? ',' : ' ') << '\n';


grahamsedman · 2026-05-29T19:11:23Z

Hi,

The ../tscore/include/ParseRules.h and ../tscore/ParseRules.cc are currently in my in tray to bring up the code quality to meet modern C++ standards and Mozilla style guide if followed. I've noticed a mix of C style code and C++ code, so I changed that to be idiomatic C++ code.

Regards

Graham

zwoop · 2026-06-05T15:22:22Z

[approve ci]

bryancall

Thanks for taking this on, but this cannot build in its current form, which is why 12 of 15 checks are failing. I will do a full review once CI is green. A few things I already see that need to happen first:

The wholesale identifier rename is the root cause. ParseRules.h only declares the snake_case members (is_char, is_upalpha, ... ink_tolower, ink_toupper) and the is_*_BIT macros. IsChar, InkTolower, IS_CHAR_BIT, etc. do not exist anywhere, so main() references dozens of undeclared names. Either revert the renames in this file, or rename in ParseRules.h and every consumer in the same change (that is a much larger change than one file).
Dropping #define COMPILE_PARSE_RULES before the ParseRules.h include breaks the generator. Without it the classifiers take the placeholder-table branch (all zeros), so the output tables would be wrong, and the placeholder global was also renamed here, which is a link error. Restore the define.
ParseRules::is_pchar takes const char *. Passing unsigned char* will not compile. Use a char buffer with an explicit cast.
Emitting (uint8_t)NN into the generated tables narrows on signed-char platforms for values >= 128, since those files initialize const char[256] arrays in ParseRules.cc. Emit static_cast<char>(NN) instead. This also contradicts the stated goal of fixing signed-char narrowing.
std::hex and std::setfill('0') are sticky, so from the second iteration on the decimal index in the /* NNN */ comment prints as zero-padded hex. Reset with std::dec and std::setfill(' ') before printing the index.

Two more worth folding in: after each write loop, check the stream state and flush before returning success. Right now an ENOSPC or mid-write failure still exits 0 and leaves a truncated table that gets compiled straight into the runtime, with no build error. And these generated tables have no direct test; an all-256-byte oracle test for ink_toupper/ink_tolower and each is_* classifier would catch exactly the high-bit regressions this refactor risks, and would have caught several of the issues above.

Please get it compiling and CI green, then I will review in full.

Incorporate latest upstream changes from master to resolve potential merge conflicts and ensure feature branch stays current with the project's master branch.

grahamsedman changed the title ~~feat: Refactor CompileParseRules.cc for type safety, modern C++, and documentation~~ refactor: Refactor CompileParseRules.cc for type safety, modern C++, and documentation May 23, 2026

grahamsedman force-pushed the fix/src-tscore-compile-parse-rules-errors branch 7 times, most recently from 282d691 to fbd054e Compare May 25, 2026 00:33

grahamsedman added 4 commits May 25, 2026 10:43

grahamsedman force-pushed the fix/src-tscore-compile-parse-rules-errors branch from fbd054e to 7a47a4f Compare May 25, 2026 09:56

grahamsedman force-pushed the fix/src-tscore-compile-parse-rules-errors branch from 7a47a4f to 2ce2079 Compare May 25, 2026 10:37

chore: merge master into fix/src-tscore-compile-parse-rules-errors

283587c

Incorporate latest upstream changes from master to resolve potential merge conflicts and ensure feature branch stays current with the project's master branch.

masaori335 assigned grahamsedman May 29, 2026

masaori335 added the Cleanup label May 29, 2026

masaori335 added this to the 11.0.0 milestone May 29, 2026

masaori335 added this to ATS v10.2.x May 29, 2026

masaori335 requested a review from Copilot May 29, 2026 04:51

Copilot started reviewing on behalf of masaori335 May 29, 2026 04:52 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

bryancall self-requested a review June 1, 2026 22:02

bryancall reviewed Jul 13, 2026

View reviewed changes

chore: merge master fix/src-tscore-compile-parse-rules-errors

524c682

Incorporate latest upstream changes from master to resolve potential merge conflicts and ensure feature branch stays current with the project's master branch.

grahamsedman force-pushed the fix/src-tscore-compile-parse-rules-errors branch from fc66ce9 to 524c682 Compare July 16, 2026 13:18

cmcfarlen moved this to For v10.2.1 in ATS v10.2.x Aug 5, 2026

Navigation Menu

Uh oh!

refactor: Refactor CompileParseRules.cc for type safety, modern C++, and documentation - #13196

refactor: Refactor CompileParseRules.cc for type safety, modern C++, and documentation#13196
grahamsedman wants to merge 8 commits into
apache:masterfrom
grahamsedman:fix/src-tscore-compile-parse-rules-errors

grahamsedman commented May 23, 2026 •

edited

Loading

Uh oh!

grahamsedman commented May 25, 2026 •

edited

Loading

Uh oh!

masaori335 commented May 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

grahamsedman commented May 29, 2026

Uh oh!

zwoop commented Jun 5, 2026

Uh oh!

bryancall left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		#include <cstdio>
		#include <cctype>

Uh oh!

Conversation

grahamsedman commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 Summary

✅ Problems Addressed

🔧 Key Changes

1. File Header & Includes

2. Data Types

3. uint_to_binary Function

4. File I/O

5. Doxygen Documentation

🧪 Testing

Uh oh!

grahamsedman commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masaori335 commented May 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

grahamsedman commented May 29, 2026

Uh oh!

zwoop commented Jun 5, 2026

Uh oh!

bryancall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

grahamsedman commented May 23, 2026 •

edited

Loading

3. `uint_to_binary` Function

grahamsedman commented May 25, 2026 •

edited

Loading