envoyproxy / envoyproxy/envoy

GeoIP LookupResult is always const, inhibiting move semantics

Open
#46,531 6 comments 0 reactions 0 assignees View on GitHub
area/geoip enhancement stale
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.