Extract duplicate parseLine() function to shared utility header#2467
Conversation
|
Let me know if you have any query regarding changes or need a further change |
There was a problem hiding this comment.
Pull request overview
Refactors duplicated HTTP header-line parsing helpers into a shared drogon::utils header so multipart parsing code can reuse the same implementation.
Changes:
- Added
lib/inc/drogon/utils/ParsingUtils.hwith inlinestartsWith(),startsWithIgnoreCase(), andparseLine(). - Removed the local static helper implementations from
lib/src/MultipartStreamParser.ccand switched it to include the new header.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/src/MultipartStreamParser.cc | Replaced local parsing helpers with shared utilities via new include. |
| lib/inc/drogon/utils/ParsingUtils.h | Introduces shared inline parsing helpers intended for reuse across parsers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| #pragma once | ||
|
|
||
| #include <string_view> | ||
| #include <utility> | ||
| #include <cctype> | ||
|
|
lib/src/MultipartStreamParser.cc
Outdated
| using namespace drogon; | ||
|
|
||
| static bool startsWith(const std::string_view &a, const std::string_view &b) | ||
| { | ||
| if (a.size() < b.size()) | ||
| { | ||
| return false; | ||
| } | ||
| for (size_t i = 0; i < b.size(); i++) | ||
| { | ||
| if (a[i] != b[i]) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| static bool startsWithIgnoreCase(const std::string_view &a, | ||
| const std::string_view &b) | ||
| { | ||
| if (a.size() < b.size()) | ||
| { | ||
| return false; | ||
| } | ||
| for (size_t i = 0; i < b.size(); i++) | ||
| { | ||
| if (::tolower(a[i]) != ::tolower(b[i])) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
| using namespace drogon::utils; |
lib/src/MultipartStreamParser.cc
Outdated
| #include "MultipartStreamParser.h" | ||
| #include <cassert> | ||
| #include <drogon/utils/ParsingUtils.h> |
lib/inc/drogon/utils/ParsingUtils.h
Outdated
| } | ||
| for (size_t i = 0; i < b.size(); i++) | ||
| { | ||
| if (::tolower(a[i]) != ::tolower(b[i])) |
|
@AzmeerX Thanks for the patch, but why can't I find a duplicate parseLine() function in the HttpRequestParser.cc file? |
|
Sorry the duplicate was in MultiPart.cc not HttpRequestParser.cc. i've replaced that local parseLine() with the shared helpers in ParsingUtils.h, and also updated the PR description. |
|
@an-tao are these changes good for merge? |
|
@an-tao Hey, I noticed a small TODO in lib/src/SharedLibManager.cc around handling the return value of safeExec(genArgs) when generating views. Can I work on that one? |
This was marked with a TODO comment noting the duplication.
The parseLine() function for parsing HTTP header lines was duplicated:
Changes Made
Extracted the duplicate functions (
parseLine(),startsWith(),startsWithIgnoreCase()) into a shared utility header:Verified compilation with this g++ -std=c++17 -I./lib/inc -c lib/src/MultipartStreamParser.cc
All function calls still work correctly; No changes to existing functionality—refactoring only