Wrap RegexMatchAndSubstitute in a class#43802
Wrap RegexMatchAndSubstitute in a class#43802ravenblackx wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/43802/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
@kyessenov I picked you to review because you've been doing string_view cleanups which feels like a similar sort of thing. :) |
|
/retest |
|
|
||
| // If the proto has no pattern, returns a null RegexReplace. | ||
| static absl::StatusOr<RegexReplace> | ||
| create(Regex::Engine& engine, const ::envoy::type::matcher::v3::RegexMatchAndSubstitute& proto); |
There was a problem hiding this comment.
I think this would be cleaner as absl::StatusOr<absl::optional<RegexReplace>>? Then you don't have to handle special IsNull case and just store optional in-place.
There was a problem hiding this comment.
Slightly cleaner, but an optional makes it sizeof(RegexReplace) + sizeof(bool) is why I went this way. But fair enough, maybe linter-requiring clients to check is valuable enough to make it worth that little cost.
This also looked like it would make sense to transform optional into OptRef for the getter function to make sense, which surprisingly didn't already exist as a thing. Ideally I would implement this as an implicit constructor of OptRef since implicit conversion for this makes sense, but (after hours of trying to figure out what the problem was) there's an issue that you can do (and in places we do)
class Foo; // forward-declared
OptRef<Foo> x;
but you can't do
class Foo; // forward-declared
void someFn(absl::optional<Foo>& x) {...}
because operating on optional requires knowing the full non-abstract type. So you can't include an abstract constructor or even a static fromOptional member in OptRef, it has to be a separate helper function, yuck. (Also, two of them, to work with const.) And then even that's a problem because there's four combinations, const optional<const T>.
So I ended up just not doing OptRef, and having those getters return a reference to the existing optional, which is at least simple. What a timesink this was.
| class RegexReplace { | ||
| public: | ||
| RegexReplace() = default; | ||
| RegexReplace(Regex::CompiledMatcherPtr regex, std::string&& substitution) |
There was a problem hiding this comment.
string_view instead of string&&. It works the same and more general.
There was a problem hiding this comment.
string_view works the same as const std::string& - std::string&& is different in that it allows for creating an instance without performing a copy, when the caller doesn't need to keep a copy (and requires the caller to explicitly make the copy if they do need to keep a copy.)
So this is a bit more flexible for performance, at the cost of being slightly more verbose at the callsite (either explicit move or explicit copy) - which for most cases is just going to be the one instance in create anyway; all existing clients don't even use the constructor directly, but it seems reasonable to leave it open to direct construction.
| regex_rewrite_substitution_ = rewrite_spec.substitution(); | ||
| auto regex_replace_or = Matcher::RegexReplace::create(regex_engine, rewrite_spec); | ||
| SET_AND_RETURN_IF_NOT_OK(regex_replace_or.status(), creation_status); | ||
| regex_replace_ = std::move(regex_replace_or.value()); |
There was a problem hiding this comment.
nit: style guide recommends move(regex_replace_or).value() because then outer object is made invalid. That prevents subtle errors when touching it later.
|
/wait |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
2 participants
Commit Message: Wrap RegexMatchAndSubstitute in a class
Additional Description: The proto
RegexMatchAndSubstitutehas repeated "copy this into two member variables and then manage the two variables" implementation. So that I don't have to add another identical implementation, this PR wraps the conversion from that proto message to a single object for performing regex substitution, to replace those two variables.I put it in the Matcher namespace simply because the proto message is in a Matcher namespace.
Note: a thing that I noticed while creating this is that the implementation of regex replaceAll performs a frequently-unnecessary string copy - feels like there should be a
replaceAllInPlaceoption that takes a stringref and just callsGlobalReplace. But because of the abstraction layers adding that might be a bit of a pain.Risk Level: Small, change should be a no-op behavior-wise.
Testing: Existing tests of use-locations still pass, added explicit tests for the new class.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a