PR Replacement #13185: refactor : Fix compilation errors in TextView.h by removing ambiguous constructor overloads and revised TextView.cc header includes#13187
Conversation
Add missing standard library headers required for compilation, such as <algorithm>, <cstring>, <cctype>, <strings.h>, <sys/types.h>, <ostream>, and <limits>. Reorder existing includes for better organisation. Apply consistent indentation and formatting style (placing opening braces on new lines) to the entire file to improve readability and adherence to style guidelines. Remove redundant TextView constructor overloads taking unsigned and ssize_t to simplify the interface and reduce ambiguity. Update namespace closing comments to reflect the new brace style.
Update the source code style and remove unnecessary header dependencies. * Adjust indentation for the contents of the swoc and SWOC_VERSION_NS namespaces. * Move opening braces for functions and namespaces to new lines. * Remove unused <cctype> and <sstream> includes.
|
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 The tarball hosted at Could a maintainer please update line 44 in Thank you! |
brbzull0
left a comment
There was a problem hiding this comment.
Hi.
Aprox ~10 line change burried into 2k lines changes? very hard to review.
|
Note, we see this in the CI failures: |
Incorporate latest upstream changes from master to resolve potential merge conflicts and ensure feature branch stays current with the project's master branch.
|
Hi, The reason for the 2k code changes is due to I formatted the file with clang format and it has been committed not been well formatted, so I fixed it. The constructor code block section near the top of the file is the place to look and the current version on github look at this code block starting at line 1121. As explained I removed some code to make it compile and it does after fixing this issue. Clangd lints this error too. Regards Graham |
I'd definitelly not have both in the same PR. Formatting is different because that it is/was originally an external library: libswoc It's excluded from the Format AFAIK, so it has his own formating style. |
|
Hi Everyone, Thank you for your review. I appreciate you flagging the formatting concern, and I acknowledge I should not have mixed clang-format changes with the functional fix in this PR. You're absolutely right that libswoc, as an external library, maintains its own formatting style and should be excluded from ATS formatting rules. I will revert all formatting changes from this PR and retain only the minimal functional fix that resolves the compile error. However, your point about the code existing in multiple locations is critical. My investigation confirms the confusion:
The core issue: The compile fix I’m proposing affects code duplicated across these repositories. To properly address this:
The current state—with code duplicated across repositories—creates exactly the confusion we’re seeing. I’d be happy to create a separate issue to track this synchronisation problem. Would you prefer I proceed with Option A or Option B for the longer-term fix? For this PR, I’ll focus solely on the minimal compile fix without any formatting changes. Best regards, |
Note: This replaces PR #13185. The branch has been renamed to
fix/lib-swoc-textview-constructorsto maintain consistent local branch organisation for my own folk.This PR fixes build failures occurring on 32-bit architectures (specifically
arm-linux-gnueabihf). Ensure both 32 bit and 64 bit targets are catered for in the refactor.Problem
On 32-bit systems,
size_tis defined asunsigned intandssize_tis defined asint. TheTextViewclass currently defines distinct constructor overloads forsize_t,unsigned,ssize_t, andint.This results in duplicate function signatures on 32-bit builds, causing the compiler to throw
constructor cannot be redeclarederrors.Errors observed:
Solution
To resolve this ambiguity, this PR refactors the constructor overloads to rely only on
size_tandint.TextView(char const *ptr, unsigned n)TextView(char const *ptr, ssize_t n)TextView(char const *ptr, size_t n)(Coversunsignedon 32-bit andsize_ton 64-bit)TextView(char const *ptr, int n)(Coversssize_ton 32-bit andinton 64-bit)The logic within the retained constructors (specifically handling
n < 0forint) remains unchanged, ensuring functional parity across architectures.Changes
Checklist:
Compilation Errors
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:122:13: error: constructor cannot be redeclared
122 | constexpr TextView(char const *ptr, unsigned n) noexcept;
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:115:13: note: previous declaration is here
115 | constexpr TextView(char const *ptr, size_t n) noexcept;
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:142:13: error: constructor cannot be redeclared
142 | constexpr TextView(char const *ptr, int n) noexcept;
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:132:13: note: previous declaration is here
132 | constexpr TextView(char const *ptr, ssize_t n) noexcept;
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:1128:28: error: redefinition of 'TextView'
1128 | inline constexpr TextView::TextView(const char *ptr, unsigned n) noexcept : super_type(ptr, size_t(n)) {}
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:1126:28: note: previous definition is here
1126 | inline constexpr TextView::TextView(const char *ptr, size_t n) noexcept
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:1131:28: error: redefinition of 'TextView'
1131 | inline constexpr TextView::TextView(const char *ptr, int n) noexcept
| ^
/home/grahamsedman/Projects/trafficserver/lib/swoc/include/swoc/TextView.h:1129:28: note: previous definition is here
1129 | inline constexpr TextView::TextView(const char *ptr, ssize_t n) noexcept