Skip to content

clear up plugin string usage ambiguities reported by gcc#10942

Merged
traeak merged 1 commit intoapache:masterfrom
traeak:gcc_strings
Dec 18, 2023
Merged

clear up plugin string usage ambiguities reported by gcc#10942
traeak merged 1 commit intoapache:masterfrom
traeak:gcc_strings

Conversation

@traeak
Copy link
Contributor

@traeak traeak commented Dec 18, 2023

This clears up some warnings with CI gcc-12 release branch builds.
tested with ubuntu:23.04 CI image with gcc/release combination.

@traeak traeak self-assigned this Dec 18, 2023
@traeak traeak added Plugins Build work related to build configuration or environment labels Dec 18, 2023
@@ -117,12 +117,15 @@ template <class T> class RateLimiter
memset(_metrics, 0, sizeof(_metrics));

std::string metric_prefix = prefix;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you use the std::string::reserve() member function for these so there will be only one heap allocation?

Copy link
Contributor Author

@traeak traeak Dec 18, 2023

Choose a reason for hiding this comment

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

Should you use the std::string::reserve() member function for these so there will be only one heap allocation?

This function is "initializeMetrics" which is only called during plugin initialization. And there's more logic below in the function for building this metric_prefix string. You might save one heap allocation with a reserve for something not in a performance sensitive place with probably too high a maintenance cost.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK it looked like a performance PR to me. What are the ambiguities in the current code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it's due to the std::string/std::string_view handling (circular dependency hell) where the compiler might get confused trying to figure out how to deal with the first argument in the following (for example):

result += "/" + somestringfunc();

@traeak traeak changed the title clear up plugin string usage ambiguities clear up plugin string usage ambiguities reported by gcc Dec 18, 2023
@traeak traeak merged commit 05113dc into apache:master Dec 18, 2023
phongn pushed a commit to phongn/trafficserver that referenced this pull request Feb 1, 2024
@zwoop zwoop added this to the 10.0.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build work related to build configuration or environment Plugins

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants