-
Notifications
You must be signed in to change notification settings - Fork 5.5k
style: convention for optional reference wrapper type alias #9725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
78c9644
b37f8fd
5a97b88
a9883ad
db259cc
e0b7af5
ddbfb7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,9 @@ class Histogram; | |
| class Scope; | ||
| class NullGaugeImpl; | ||
|
|
||
| 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>>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be CounterOptConstRef per your style section?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I should definitely follow my rules 😅. I checked all the other aliases for correctness.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| using GaugeOptRef = absl::optional<std::reference_wrapper<const Gauge>>; | ||
| using HistogramOptRef = absl::optional<std::reference_wrapper<const Histogram>>; | ||
| using ScopePtr = std::unique_ptr<Scope>; | ||
| using ScopeSharedPtr = std::shared_ptr<Scope>; | ||
|
|
||
|
|
@@ -98,20 +98,20 @@ class Scope { | |
| * @param The name of the stat, obtained from the SymbolTable. | ||
| * @return a reference to a counter within the scope's namespace, if it exists. | ||
| */ | ||
| virtual OptionalCounter findCounter(StatName name) const PURE; | ||
| virtual CounterOptRef findCounter(StatName name) const PURE; | ||
|
|
||
| /** | ||
| * @param The name of the stat, obtained from the SymbolTable. | ||
| * @return a reference to a gauge within the scope's namespace, if it exists. | ||
| */ | ||
| virtual OptionalGauge findGauge(StatName name) const PURE; | ||
| virtual GaugeOptRef findGauge(StatName name) const PURE; | ||
|
|
||
| /** | ||
| * @param The name of the stat, obtained from the SymbolTable. | ||
| * @return a reference to a histogram within the scope's namespace, if it | ||
| * exists. | ||
| */ | ||
| virtual OptionalHistogram findHistogram(StatName name) const PURE; | ||
| virtual HistogramOptRef findHistogram(StatName name) const PURE; | ||
|
|
||
| /** | ||
| * @return a reference to the symbol table. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably expand the language on line 41 beyond "smart pointers".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekargueta not sure what you mean? I see these two as different bullet points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, missed the un-indentation on 46 to make it a new section 👍