GeoIP LookupResult is always const, inhibiting move semantics
- Dominant language
- C++
- Stars
- 28.9k
- Forks
- 5.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 437
Description
*Description*:
In `envoy/geoip/geoip_provider_driver.h`, the type `LookupResult` is defined as an alias to `const absl::flat_hash_map`.
The type is usually passed around by rvalue-reference:
```
// from source/extensions/filters/http/geoip/geoip_filter.cc
driver_->lookup(
Geolocation::LookupRequest{std::move(remote_address)},
[self, &dispatcher = decoder_callbacks_->dispatcher()](Geolocation::LookupResult&& result) {
dispatcher.post([self, result]() {
if (GeoipFilterSharedPtr filter = self.lock()) {
filter->onLookupComplete(std::move(result));
}
});
});
```
Hiding the `const` in the type like this makes the code confusing and makes it impossible to move a `LookupResult`. In the code above for instance, the `result` could be moved into the lambda instead of copied.
It can also trigger linters that check for unnecessary uses of `std::move`, for people who have those enabled.
I would be willing to refactor this if you're open to it. I think it would not be a big change, just remove the `const` and maybe add some `move`s to take advantage of it. But I thought I'd ask in case this `const` is intended like this.
Contributor guide
Research direction
Start with envoy/geoip/geoip_provider_driver.h and trace LookupResult through the callback in source/extensions/filters/http/geoip/geoip_filter.cc. Check how the alias and captured result are copied or moved, then inspect the related GeoIP call sites for consistency. Done means the result is no longer inherently const and the affected flow no longer performs avoidable copies without changing behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, performance
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100