Skip to content

Conversation

JakeChampion
Copy link
Contributor

I've been working on the compress plugin quite a lot recently and came to learn about libswoc from another pull-request i was doing, I thought it was a really good library and that this plugin might benefit from using more of it's utilities

most of these commits are likely able to be landed individually, if it would be better to open each one as a separate pull-request/patch i can do that ☺️

@bryancall bryancall requested a review from bneradt October 6, 2025 22:10
@bryancall bryancall added the compress compress plugin label Oct 6, 2025
@bryancall bryancall added this to the 10.2.0 milestone Oct 6, 2025
@bneradt
Copy link
Contributor

bneradt commented Oct 6, 2025

Good updates. Looks like there's a couple wrinkles to iron out though:

https://ci.trafficserver.apache.org/job/Github_Builds/job/autest/31205/console

../plugins/compress/configuration.cc:258:1: error: call of overloaded 'Lexicon(<brace-enclosed initializer list>)' is ambiguous
  258 | };
      | ^
../plugins/compress/configuration.cc:258:1: note: there are 5 candidates
In file included from ../plugins/compress/configuration.cc:36:
../lib/swoc/include/swoc/Lexicon.h:608:23: note: candidate 1: 'swoc::_1_5_14::Lexicon<E>::Lexicon(with, Default, Default) [with E = Gzip::RangeRequestCtrl; with = const std::initializer_list<swoc::_1_5_14::detail::lexicon_pair_type<Gzip::RangeRequestCtrl> >&; Default = std::variant<std::monostate, Gzip::RangeRequestCtrl, swoc::_1_5_14::TextView, std::function<Gzip::RangeRequestCtrl(swoc::_1_5_14::TextView)>, std::function<swoc::_1_5_14::TextView(Gzip::RangeRequestCtrl)> >]'
  608 | template <typename E> Lexicon<E>::Lexicon(with items, Default handler_1, Default handler_2) {
      |                       ^~~~~~~~~~
../lib/swoc/include/swoc/Lexicon.h:598:23: note: candidate 2: 'swoc::_1_5_14::Lexicon<E>::Lexicon(with_multi, Default, Default) [with E = Gzip::RangeRequestCtrl; with_multi = const std::initializer_list<swoc::_1_5_14::Lexicon<Gzip::RangeRequestCtrl>::Definition>&; Default = std::variant<std::monostate, Gzip::RangeRequestCtrl, swoc::_1_5_14::TextView, std::function<Gzip::RangeRequestCtrl(swoc::_1_5_14::TextView)>, std::function<swoc::_1_5_14::TextView(Gzip::RangeRequestCtrl)> >]'
  598 | template <typename E> Lexicon<E>::Lexicon(with_multi items, Default handler_1, Default handler_2) {
      |                       ^~~~~~~~~~
In file included from ../include/ts/ts.h:39,
                 from ../plugins/compress/configuration.h:30,
                 from ../plugins/compress/configuration.cc:25:


../plugins/compress/configuration.cc: In static member function 'static Gzip::Configuration* Gzip::Configuration::Parse(const char*)':
../plugins/compress/debug_macros.h:48:31: error: format '%d' expects argument of type 'int', but argument 6 has type 'Gzip::ParserState' [-Werror=format=]
   48 |     Dbg(compress_ns::dbg_ctl, "WARNING: " fmt, ##args); \
../include/tsutil/DbgCtl.h:169:90: note: in definition of macro 'DbgPrint'
  169 | #define DbgPrint(CTL, ...) (DbgCtl::print((CTL).tag(), __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__))
      |                                                                                          ^~~~~~~~~~~
../plugins/compress/debug_macros.h:48:5: note: in expansion of macro 'Dbg'
   48 |     Dbg(compress_ns::dbg_ctl, "WARNING: " fmt, ##args); \
      |     ^~~
../plugins/compress/configuration.cc:414:5: note: in expansion of macro 'warning'
  414 |     warning("the parser state indicates that data was expected when it reached the end of the file (%d)", state);
      |     ^~~~~~~
../plugins/compress/configuration.cc:414:102: note: format string is defined here
  414 |     warning("the parser state indicates that data was expected when it reached the end of the file (%d)", state);
      |                                                                                                     ~^
      |                                                                                                      |
      |        

Comment on lines +155 to 156
return compressible_status_codes_.contains(status_code);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice :)

@JakeChampion JakeChampion requested a review from bneradt October 7, 2025 15:30
@JakeChampion
Copy link
Contributor Author

@bneradt all looking good now on ci :D

Comment on lines 42 to +52
{
using namespace std;

void
ltrim_if(string &s, int (*fp)(int))
inline auto
make_char_predicate(int (*fp)(int))
{
for (size_t i = 0; i < s.size();) {
if (fp(s[i])) {
s.erase(i, 1);
} else {
break;
}
}
return [fp](char c) -> bool { return fp(static_cast<unsigned char>(c)) != 0; };
}

void
rtrim_if(string &s, int (*fp)(int))
swoc::TextView
extractFirstToken(swoc::TextView &view, int (*fp)(int))
{
for (ssize_t i = static_cast<ssize_t>(s.size()) - 1; i >= 0; i--) {
if (fp(s[i])) {
s.erase(i, 1);
} else {
break;
}
}
}

void
trim_if(string &s, int (*fp)(int))
{
ltrim_if(s, fp);
rtrim_if(s, fp);
}

string
extractFirstToken(string &s, int (*fp)(int))
{
int startTok{-1}, endTok{-1}, idx{0};

for (;; ++idx) {
if (idx == int(s.length())) {
if (endTok < 0) {
endTok = idx;
}
break;
} else if (fp(s[idx])) {
if ((startTok >= 0) and (endTok < 0)) {
endTok = idx;
}
} else if (endTok > 0) {
break;
} else if (startTok < 0) {
startTok = idx;
}
}
auto predicate = make_char_predicate(fp);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need make_char_predicate. You can just pass fp directly to your *_if TextView functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compress compress plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants