style: convention for optional reference wrapper type alias #9725
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
|
@jmarantz this is kind of related to #9710. I was chatting with @derekargueta about placing format checking for smart pointer and for this type of aliases. I can create an issue to track if you think it is worth to enforce. |
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
| @@ -43,6 +43,9 @@ | |||
| * `using BarSharedPtr = std::shared_ptr<Bar>;` | |||
| * `using BlahConstSharedPtr = std::shared_ptr<const Blah>;` | |||
| * Regular pointers (e.g. `int* foo`) should not be type aliased. | |||
| * `absl::optional<std::reference_wrapper<T>> is type aliased: | |||
| * `using FooOptRef = absl::optional<std::reference_wrapper<T>>;` | |||
| * `using FooOptConstRef = absl::optional<std::reference_wrapper<T>>;` | |||
There was a problem hiding this comment.
We should probably expand the language on line 41 beyond "smart pointers".
There was a problem hiding this comment.
@derekargueta not sure what you mean? I see these two as different bullet points.
There was a problem hiding this comment.
oh sorry, missed the un-indentation on 46 to make it a new section 👍
Signed-off-by: Jose Nino <jnino@lyft.com>
include/envoy/stats/scope.h
Outdated
| using OptionalCounter = absl::optional<std::reference_wrapper<const Counter>>; | ||
| using OptionalGauge = absl::optional<std::reference_wrapper<const Gauge>>; | ||
| using OptionalHistogram = absl::optional<std::reference_wrapper<const Histogram>>; | ||
| using CounterOptRef = absl::optional<std::reference_wrapper<const Counter>>; |
There was a problem hiding this comment.
should this be CounterOptConstRef per your style section?
There was a problem hiding this comment.
Yeah, I should definitely follow my rules 😅. I checked all the other aliases for correctness.
There was a problem hiding this comment.
Overall I would like the aliases to be CI checked in our formatting run. I will open an issue for that if someone wants to pick up
Signed-off-by: Jose Nino <jnino@lyft.com>
Description: noticed in #9516 that we did not have a convention around aliasing
absl::optional<std::reference_wrapper<T>>. This PR proposes one.Risk Level: low. compiles.
Testing: compilation and test suite ran.
Docs Changes: added the convention in STYLE.md
Signed-off-by: Jose Nino jnino@lyft.com