Skip to content

Commit

Permalink
Use const_str() not _str() when generating point of use literals
Browse files Browse the repository at this point in the history 
to be used in str_match() / str_casematch(). As discussed with
@liviuchircu here 0147baa21072c5b9#commitcomment-46519279
it saves us some code size and also should have at least some
positive performance effect by not saving 2 values into memory
that nobody reads.
  • Loading branch information
@sobomax
sobomax committed Jan 29, 2021
1 parent ce8b994 commit fc22e17
Show file tree
Hide file tree
Showing 8 changed files with 100 additions and 100 deletions.
2 changes: 1 addition & 1 deletion lib/reg/pn.c
View file
Expand Up @@ -545,7 +545,7 @@ static struct usr_avp *pn_trim_pn_params(evi_params_t *params)
}

/* the Contact URI is the only EVI param we're interested in */
if (str_match(&p->name, _str(UL_EV_PARAM_CT_URI)) &&
if (str_match(&p->name, const_str(UL_EV_PARAM_CT_URI)) &&
pn_has_uri_params(&p->val.s, &puri)) {
if (pn_remove_uri_params(&puri, p->val.s.len, &_sval) != 0) {
LM_ERR("failed to remove PN params from Contact '%.*s'\n",
Expand Down
82 changes: 41 additions & 41 deletions lib/test/test_csv.c
View file
Expand Up @@ -30,43 +30,43 @@ void test_csv_simple(void)
csv_record *ret;

ret = parse_csv_record(_str(""));
ok(str_match(&ret->s, _str("")));
ok(str_match(&ret->s, const_str("")));
ok(!ret->next);
free_csv_record(ret);

ret = parse_csv_record(_str("\t\r\n "));
ok(str_match(&ret->s, _str("")));
ok(str_match(&ret->s, const_str("")));
ok(!ret->next);
free_csv_record(ret);

ret = parse_csv_record(_str(" a b "));
ok(str_match(&ret->s, _str("a b")));
ok(str_match(&ret->s, const_str("a b")));
ok(!ret->next);
free_csv_record(ret);

ret = parse_csv_record(_str(" a , "));
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str("")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str("")));
ok(!ret->next->next);
free_csv_record(ret);

ret = parse_csv_record(_str(" , a "));
ok(str_match(&ret->s, _str("")));
ok(str_match(&ret->next->s, _str("a")));
ok(str_match(&ret->s, const_str("")));
ok(str_match(&ret->next->s, const_str("a")));
ok(!ret->next->next);
free_csv_record(ret);

ret = parse_csv_record(_str("a,b,c"));
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str("b")));
ok(str_match(&ret->next->next->s, _str("c")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str("b")));
ok(str_match(&ret->next->next->s, const_str("c")));
ok(!ret->next->next->next);
free_csv_record(ret);

ret = parse_csv_record(_str(" a, \"b\" , \" c "));
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str("\"b\"")));
ok(str_match(&ret->next->next->s, _str("\" c")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str("\"b\"")));
ok(str_match(&ret->next->next->s, const_str("\" c")));
ok(!ret->next->next->next);
free_csv_record(ret);
}
Expand All @@ -84,50 +84,50 @@ void test_csv_rfc_4180(void)
ok(!_parse_csv_record(_str("a,b,c\t"), CSV_RFC_4180));

ret = _parse_csv_record(_str(""), CSV_RFC_4180);
ok(str_match(&ret->s, _str("")));
ok(str_match(&ret->s, const_str("")));
ok(!ret->next);
free_csv_record(ret);

ret = _parse_csv_record(_str(" a "), CSV_RFC_4180);
ok(str_match(&ret->s, _str(" a ")));
ok(str_match(&ret->s, const_str(" a ")));
ok(!ret->next);
free_csv_record(ret);

ret = _parse_csv_record(_str(" \r\n"), CSV_RFC_4180);
ok(str_match(&ret->s, _str(" ")));
ok(str_match(&ret->s, const_str(" ")));
ok(!ret->next);
free_csv_record(ret);

ret = _parse_csv_record(_str(","), CSV_RFC_4180);
ok(str_match(&ret->s, _str("")));
ok(str_match(&ret->next->s, _str("")));
ok(str_match(&ret->s, const_str("")));
ok(str_match(&ret->next->s, const_str("")));
ok(!ret->next->next);
free_csv_record(ret);

ret = _parse_csv_record(_str("a,,,"), CSV_RFC_4180);
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str("")));
ok(str_match(&ret->next->next->s, _str("")));
ok(str_match(&ret->next->next->next->s, _str("")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str("")));
ok(str_match(&ret->next->next->s, const_str("")));
ok(str_match(&ret->next->next->next->s, const_str("")));
ok(!ret->next->next->next->next);
free_csv_record(ret);

ret = _parse_csv_record(_str(" , a "), CSV_RFC_4180);
ok(str_match(&ret->s, _str(" ")));
ok(str_match(&ret->next->s, _str(" a ")));
ok(str_match(&ret->s, const_str(" ")));
ok(str_match(&ret->next->s, const_str(" a ")));
ok(!ret->next->next);
free_csv_record(ret);

ret = _parse_csv_record(_str(" a , \r\n"), CSV_RFC_4180);
ok(str_match(&ret->s, _str(" a ")));
ok(str_match(&ret->next->s, _str(" ")));
ok(str_match(&ret->s, const_str(" a ")));
ok(str_match(&ret->next->s, const_str(" ")));
ok(!ret->next->next);
free_csv_record(ret);

ret = _parse_csv_record(_str("a,b,c"), CSV_RFC_4180);
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str("b")));
ok(str_match(&ret->next->next->s, _str("c")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str("b")));
ok(str_match(&ret->next->next->s, const_str("c")));
ok(!ret->next->next->next);
free_csv_record(ret);

Expand All @@ -137,30 +137,30 @@ void test_csv_rfc_4180(void)
ok(!_parse_csv_record(_str("\"a\"a,b"), CSV_RFC_4180));

ret = _parse_csv_record(_str("\"a\""), CSV_RFC_4180);
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->s, const_str("a")));
ok(!ret->next);
free_csv_record(ret);

ret = _parse_csv_record(_str("realm=\"etc.example.com\","
"nonce=\"B5CFDSFDGD14992F\",opaque=\"opaq\","
"qop=\"auth\",algorithm=MD5"), CSV_RFC_4180);
ok(str_match(&ret->s, _str("realm=\"etc.example.com\"")));
ok(str_match(&ret->next->s, _str("nonce=\"B5CFDSFDGD14992F\"")));
ok(str_match(&ret->next->next->s, _str("opaque=\"opaq\"")));
ok(str_match(&ret->next->next->next->s, _str("qop=\"auth\"")));
ok(str_match(&ret->next->next->next->next->s, _str("algorithm=MD5")));
ok(str_match(&ret->s, const_str("realm=\"etc.example.com\"")));
ok(str_match(&ret->next->s, const_str("nonce=\"B5CFDSFDGD14992F\"")));
ok(str_match(&ret->next->next->s, const_str("opaque=\"opaq\"")));
ok(str_match(&ret->next->next->next->s, const_str("qop=\"auth\"")));
ok(str_match(&ret->next->next->next->next->s, const_str("algorithm=MD5")));
ok(!ret->next->next->next->next->next);
free_csv_record(ret);

ret = _parse_csv_record(_str("a,\"\tb\""), CSV_RFC_4180);
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str("\tb")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str("\tb")));
ok(!ret->next->next);
free_csv_record(ret);

ret = _parse_csv_record(_str("\"a\", bc "), CSV_RFC_4180);
ok(str_match(&ret->s, _str("a")));
ok(str_match(&ret->next->s, _str(" bc ")));
ok(str_match(&ret->s, const_str("a")));
ok(str_match(&ret->next->s, const_str(" bc ")));
ok(!ret->next->next);
free_csv_record(ret);

Expand All @@ -171,8 +171,8 @@ void test_csv_rfc_4180(void)

ret = _parse_csv_record(_str("\"\"\"a\"\" \r\"\"\t\nb\"\" \",\"c\"\r\n"),
CSV_RFC_4180);
ok(str_match(&ret->s, _str("\"a\" \r\"\t\nb\" ")));
ok(str_match(&ret->next->s, _str("c")));
ok(str_match(&ret->s, const_str("\"a\" \r\"\t\nb\" ")));
ok(str_match(&ret->next->s, const_str("c")));
ok(!ret->next->next);
free_csv_record(ret);
}
Expand Down
6 changes: 3 additions & 3 deletions modules/dialplan/dialplan.c
View file
Expand Up @@ -266,10 +266,10 @@ static int dp_create_head(const str *in)
return 0;
}

if (str_match(&params->s, _str(PARAM_URL))) {
if (str_match(&params->s, const_str(PARAM_URL))) {
have_db_url = 1;
dp_head_insert(DP_TYPE_URL, &params->next->s, &partition);
} else if (str_match(&params->s, _str(PARAM_TABLE))) {
} else if (str_match(&params->s, const_str(PARAM_TABLE))) {
have_db_table = 1;
dp_head_insert(DP_TYPE_TABLE, &params->next->s, &partition);
} else if (!ZSTR(params->s)) {
Expand Down Expand Up @@ -359,7 +359,7 @@ static int mod_init(void)
timerec_column.len = strlen(timerec_column.s);
disabled_column.len = strlen(disabled_column.s);

if (!dp_df_head && str_match(&dp_df_part, _str(DEFAULT_PARTITION)) &&
if (!dp_df_head && str_match(&dp_df_part, const_str(DEFAULT_PARTITION)) &&
default_dp_db_url.s) {
dp_head_insert(DP_TYPE_URL, &default_dp_db_url, &dp_df_part);
dp_head_insert(DP_TYPE_TABLE, &default_dp_table, &dp_df_part);
Expand Down
2 changes: 1 addition & 1 deletion modules/dialplan/dp_db.c
View file
Expand Up @@ -176,7 +176,7 @@ int init_data(void)
}

/* was the default partition re-pointed? */
if (!str_match(&dp_df_part, _str(DEFAULT_PARTITION))) {
if (!str_match(&dp_df_part, const_str(DEFAULT_PARTITION))) {
int found = 0;

for (start = dp_hlist; start; start = start->next) {
Expand Down
6 changes: 3 additions & 3 deletions modules/fraud_detection/frd_load.c
View file
Expand Up @@ -231,9 +231,9 @@ static int create_time_rec(const str *time_start, const str *time_end,
tmrec_p trec = &trx->tr;

/* the default, "catch-all" time rec - using NULL is optimal */
if (str_match(time_start, _str("00:00")) &&
str_match(time_end, _str("23:59")) &&
str_match(week_days, _str("Mon-Sun"))) {
if (str_match(time_start, const_str("00:00")) &&
str_match(time_end, const_str("23:59")) &&
str_match(week_days, const_str("Mon-Sun"))) {
*out_rec = NULL;
return 0;
} else {
Expand Down
24 changes: 12 additions & 12 deletions parser/parse_uri.h
View file
Expand Up @@ -127,80 +127,80 @@ static inline int get_uri_param_val(const struct sip_uri *uri,
switch (param->s[0]) {
case 'p':
case 'P':
if (str_casematch(param, _str("pn-provider"))) {
if (str_casematch(param, const_str("pn-provider"))) {
*val = uri->pn_provider_val;
return 0;
}

if (str_casematch(param, _str("pn-prid"))) {
if (str_casematch(param, const_str("pn-prid"))) {
*val = uri->pn_prid_val;
return 0;
}

if (str_casematch(param, _str("pn-param"))) {
if (str_casematch(param, const_str("pn-param"))) {
*val = uri->pn_param_val;
return 0;
}

if (str_casematch(param, _str("pn-purr"))) {
if (str_casematch(param, const_str("pn-purr"))) {
*val = uri->pn_purr_val;
return 0;
}
break;

case 't':
case 'T':
if (str_casematch(param, _str("transport"))) {
if (str_casematch(param, const_str("transport"))) {
*val = uri->transport_val;
return 0;
}

if (str_casematch(param, _str("ttl"))) {
if (str_casematch(param, const_str("ttl"))) {
*val = uri->ttl_val;
return 0;
}
break;

case 'u':
case 'U':
if (str_casematch(param, _str("user"))) {
if (str_casematch(param, const_str("user"))) {
*val = uri->user_param_val;
return 0;
}
break;

case 'm':
case 'M':
if (str_casematch(param, _str("maddr"))) {
if (str_casematch(param, const_str("maddr"))) {
*val = uri->maddr_val;
return 0;
}

if (str_casematch(param, _str("method"))) {
if (str_casematch(param, const_str("method"))) {
*val = uri->method_val;
return 0;
}
break;

case 'l':
case 'L':
if (str_casematch(param, _str("lr"))) {
if (str_casematch(param, const_str("lr"))) {
*val = uri->lr_val;
return 0;
}
break;

case 'r':
case 'R':
if (str_casematch(param, _str("r2"))) {
if (str_casematch(param, const_str("r2"))) {
*val = uri->r2_val;
return 0;
}
break;

case 'g':
case 'G':
if (str_casematch(param, _str("gr"))) {
if (str_casematch(param, const_str("gr"))) {
*val = uri->gr_val;
return 0;
}
Expand Down
18 changes: 9 additions & 9 deletions parser/test/test_parse_fcaps.c
View file
Expand Up @@ -35,25 +35,25 @@ void test_parse_fcaps(void)
memset(&hf, 0, sizeof hf);
get_hdr_field(hdr->s, hdr->s + hdr->len, &hf);
ok(hf.type == HDR_FEATURE_CAPS_T, "fcaps-1");
ok(str_match(&hf.body, _str("+sip.pns=\"apns\"")), "fcaps-2");
ok(str_match(&hf.body, const_str("+sip.pns=\"apns\"")), "fcaps-2");

hdr = _str("fEATURE-cAPS:+sip.pns=\"apns\"\r\n");
memset(&hf, 0, sizeof hf);
get_hdr_field(hdr->s, hdr->s + hdr->len, &hf);
ok(hf.type == HDR_FEATURE_CAPS_T, "fcaps-3");
ok(str_match(&hf.body, _str("+sip.pns=\"apns\"")), "fcaps-4");
ok(str_match(&hf.body, const_str("+sip.pns=\"apns\"")), "fcaps-4");

hdr = _str("feature-caps: +sip.pns=\"apns\";+sip.pnsreg=\"130\"\r\n");
memset(&hf, 0, sizeof hf);
get_hdr_field(hdr->s, hdr->s + hdr->len, &hf);
ok(hf.type == HDR_FEATURE_CAPS_T, "fcaps-5");
ok(str_match(&hf.body, _str("+sip.pns=\"apns\";+sip.pnsreg=\"130\"")), "fcaps-6");
ok(str_match(&hf.body, const_str("+sip.pns=\"apns\";+sip.pnsreg=\"130\"")), "fcaps-6");
free_fcaps((fcaps_body_t**)&hf.parsed);
ok(!hf.parsed, "fcaps-7");

ok(parse_fcaps(&hf) == 0, "fcaps-8");
fcaps = (fcaps_body_t *)hf.parsed;
ok(str_match(&fcaps->pns, _str("apns")), "fcaps-9");
ok(str_match(&fcaps->pns, const_str("apns")), "fcaps-9");
free_fcaps((fcaps_body_t**)&hf.parsed);

hf.body = *_str("");
Expand All @@ -80,30 +80,30 @@ void test_parse_fcaps(void)
hf.body = *_str("+sip.pns=\"x\"");
ok(parse_fcaps(&hf) == 0, "fcaps-17");
fcaps = (fcaps_body_t *)hf.parsed;
ok(str_match(&fcaps->pns, _str("x")), "fcaps-18");
ok(str_match(&fcaps->pns, const_str("x")), "fcaps-18");
free_fcaps((fcaps_body_t**)&hf.parsed);

hf.body = *_str("+sip.pns=\"apns\";+sip.pns=130\";+sip.pns=\"fcm\"+sip.pns+sip.pns=\"x");
ok(parse_fcaps(&hf) == 0, "fcaps-19");
fcaps = (fcaps_body_t *)hf.parsed;
ok(str_match(&fcaps->pns, _str("fcm")), "fcaps-20");
ok(str_match(&fcaps->pns, const_str("fcm")), "fcaps-20");
free_fcaps((fcaps_body_t**)&hf.parsed);

hf.body = *_str("+sip.pns=\"apns\";+sip.pns=130\";+sip.pns=\"fcm\"+sip.pns+sip.pns=\"x");
ok(parse_fcaps(&hf) == 0, "fcaps-21");
fcaps = (fcaps_body_t *)hf.parsed;
ok(str_match(&fcaps->pns, _str("fcm")), "fcaps-22");
ok(str_match(&fcaps->pns, const_str("fcm")), "fcaps-22");
free_fcaps((fcaps_body_t**)&hf.parsed);

hf.body = *_str("+sip.pns=\"apns\";+sip.pns=130\";+sip.pns=\"fcm\"+sip.pns+sip.pns=\"3");
ok(parse_fcaps(&hf) == 0, "fcaps-23");
fcaps = (fcaps_body_t *)hf.parsed;
ok(str_match(&fcaps->pns, _str("fcm")), "fcaps-24");
ok(str_match(&fcaps->pns, const_str("fcm")), "fcaps-24");
free_fcaps((fcaps_body_t**)&hf.parsed);

hf.body = *_str("+sip.pns=\"apns\";+sip.pns=130\";+sip.pns=\"fcm\"+sip.pns+sip.pns=3;+sip.pns=\"webpush\"");
ok(parse_fcaps(&hf) == 0, "fcaps-25");
fcaps = (fcaps_body_t *)hf.parsed;
ok(str_match(&fcaps->pns, _str("webpush")), "fcaps-26");
ok(str_match(&fcaps->pns, const_str("webpush")), "fcaps-26");
free_fcaps((fcaps_body_t**)&hf.parsed);
}

3 comments on commit fc22e17

@liviuchircu
Copy link
Member

@liviuchircu liviuchircu commented on fc22e17 Jan 29, 2021 •
edited

Choose a reason for hiding this comment

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

Why not just alias the _str() macro to point to const_str()? The whole point was to make it a "tiny caster" macro, so you get your job done almost invisibly. So unit test readability was the primary concern -- I'd rather read useful stuff when reviewing unit tests, rather than "const const const const const const" all day long...

@sobomax
Copy link
Contributor Author

@sobomax sobomax commented on fc22e17 Jan 29, 2021 •
edited

Choose a reason for hiding this comment

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

Why not just alias the _str() macro to point to const_str()?

@liviuchircu there are still cases where _str() is quite adequate as long as it's not abused. Like when you need to convert char * or const char * into str-like struct, but you don't know the length of it. Obviously you cannot generate proper const str_const in that situation, which what makes const_str() magical. :) Plus there are lots of places where you cannot easily replace str with str_const right now without going and cleaning out all APIs in chain...

Please note that if const-portion of const_str() bothers you you can easily get around it by adding #define LSTR(x) const_str(x) at the very top of your test file, then you will be shielded from any such fallouts. :)

@sobomax
Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. By making this replacement I am also hoping that cases when people would just copy&paste wrong API (i.e. _str()) into their code would be reduced or even eliminated altogether.

Please sign in to comment.