Re: Review Request 32536: Updated variable naming style.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32536/#review80487
---
Captured my thoughts on this topic in the JIRA ticket. I think this would be a
good step towards what I think we ought to get to, which is to assign the
following groups into different naming schemes: types, local variables
(including function parameters), member variables, and member-access functions.
- Michael Park
On April 14, 2015, 1:08 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32536/
> ---
>
> (Updated April 14, 2015, 1:08 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Michael Park, Niklas
> Nielsen, and Timothy Chen.
>
>
> Bugs: MESOS-2616
> https://issues.apache.org/jira/browse/MESOS-2616
>
>
> Repository: mesos-incubating
>
>
> Description
> ---
>
> Documents the patterns we use to name variables and function arguments in our
> codebase.
>
> Leading underscores to avoid ambiguity.
> ===
>
> We use this pattern extensively in libprocess, stout and mesos, a few
> examples below.
>
> * `stout/try.hpp:105`
> ```
> Try(State _state, T* _t = NULL, const std::string& _message = "")
> : state(_state), t(_t), message(_message) {}
> ```
>
> * `process/http.hpp:480`
> ```
> URL(const std::string& _scheme,
> const std::string& _domain,
> const uint16_t _port = 80,
> const std::string& _path = "/",
> const hashmap& _query =
> (hashmap()),
> const Option& _fragment = None())
> : scheme(_scheme),
> domain(_domain),
> port(_port),
> path(_path),
> query(_query),
> fragment(_fragment) {}
> ```
>
> * `slave/containerizer/linux_launcher.cpp:56`
> ```
> LinuxLauncher::LinuxLauncher(
> const Flags& _flags,
> int _namespaces,
> const string& _hierarchy)
> : flags(_flags),
> namespaces(_namespaces),
> hierarchy(_hierarchy) {}
> ```
>
> Trailing undescores as prime symbols.
> =
>
> We use this pattern in the code, though not extensively. I would like to see
> more pass-by-value instead of creating copies from a variable passed by const
> reference.
>
> * `master.cpp:2942`
> ```
> // Create and add the slave id.
> SlaveInfo slaveInfo_ = slaveInfo;
> slaveInfo_.mutable_id()->CopyFrom(newSlaveId());
> ```
>
> * `slave.cpp:4180`
> ```
> ExecutorInfo executorInfo_ = executor->info;
> Resources resources = executorInfo_.resources();
> resources += taskInfo.resources();
> executorInfo_.mutable_resources()->CopyFrom(resources);
> ```
>
> * `status_update_manager.cpp:474`
> ```
> // Bounded exponential backoff.
> Duration duration_ =
> std::min(duration * 2, STATUS_UPDATE_RETRY_INTERVAL_MAX);
> ```
>
> * `containerizer/mesos/containerizer.cpp:109`
> ```
> // Modify the flags to include any changes to isolation.
> Flags flags_ = flags;
> flags_.isolation = isolation;
> ```
>
> Passing arguments by value.
> ===
>
> * `slave.cpp:2480`
> ```
> void Slave::statusUpdate(StatusUpdate update, const UPID& pid)
> {
> ...
> // Set the source before forwarding the status update.
> update.mutable_status()->set_source(
> pid == UPID() ? TaskStatus::SOURCE_SLAVE : TaskStatus::SOURCE_EXECUTOR);
> ...
> }
> ```
>
> * `process/metrics/timer.hpp:103`
> ```
> static void _time(Time start, Timer that)
> {
> const Time stop = Clock::now();
>
> double value;
>
> process::internal::acquire(&that.data->lock);
> {
> that.data->lastValue = T(stop - start).value();
> value = that.data->lastValue.get();
> }
> process::internal::release(&that.data->lock);
>
> that.push(value);
> }
> ```
>
>
> Diffs
> -
>
> docs/mesos-c++-style-guide.md 439fe12
>
> Diff: https://reviews.apache.org/r/32536/diff/
>
>
> Testing
> ---
>
> None (not a functional change).
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32536: Updated variable naming style.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32536/#review80015
---
Ship it!
Thanks for bringing this up Alex - I find it very clear and helpful.
- Till Toenshoff
On March 26, 2015, 4:10 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32536/
> ---
>
> (Updated March 26, 2015, 4:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Michael Park, Niklas
> Nielsen, and Timothy Chen.
>
>
> Repository: mesos-incubating
>
>
> Description
> ---
>
> Documents the patterns we use to name variables and function arguments in our
> codebase.
>
> Leading underscores to avoid ambiguity.
> ===
>
> We use this pattern extensively in libprocess, stout and mesos, a few
> examples below.
>
> * `stout/try.hpp:105`
> ```
> Try(State _state, T* _t = NULL, const std::string& _message = "")
> : state(_state), t(_t), message(_message) {}
> ```
>
> * `process/http.hpp:480`
> ```
> URL(const std::string& _scheme,
> const std::string& _domain,
> const uint16_t _port = 80,
> const std::string& _path = "/",
> const hashmap& _query =
> (hashmap()),
> const Option& _fragment = None())
> : scheme(_scheme),
> domain(_domain),
> port(_port),
> path(_path),
> query(_query),
> fragment(_fragment) {}
> ```
>
> * `slave/containerizer/linux_launcher.cpp:56`
> ```
> LinuxLauncher::LinuxLauncher(
> const Flags& _flags,
> int _namespaces,
> const string& _hierarchy)
> : flags(_flags),
> namespaces(_namespaces),
> hierarchy(_hierarchy) {}
> ```
>
> Trailing undescores as prime symbols.
> =
>
> We use this pattern in the code, though not extensively. I would like to see
> more pass-by-value instead of creating copies from a variable passed by const
> reference.
>
> * `master.cpp:2942`
> ```
> // Create and add the slave id.
> SlaveInfo slaveInfo_ = slaveInfo;
> slaveInfo_.mutable_id()->CopyFrom(newSlaveId());
> ```
>
> * `slave.cpp:4180`
> ```
> ExecutorInfo executorInfo_ = executor->info;
> Resources resources = executorInfo_.resources();
> resources += taskInfo.resources();
> executorInfo_.mutable_resources()->CopyFrom(resources);
> ```
>
> * `status_update_manager.cpp:474`
> ```
> // Bounded exponential backoff.
> Duration duration_ =
> std::min(duration * 2, STATUS_UPDATE_RETRY_INTERVAL_MAX);
> ```
>
> * `containerizer/mesos/containerizer.cpp:109`
> ```
> // Modify the flags to include any changes to isolation.
> Flags flags_ = flags;
> flags_.isolation = isolation;
> ```
>
> Passing arguments by value.
> ===
>
> * `slave.cpp:2480`
> ```
> void Slave::statusUpdate(StatusUpdate update, const UPID& pid)
> {
> ...
> // Set the source before forwarding the status update.
> update.mutable_status()->set_source(
> pid == UPID() ? TaskStatus::SOURCE_SLAVE : TaskStatus::SOURCE_EXECUTOR);
> ...
> }
> ```
>
> * `process/metrics/timer.hpp:103`
> ```
> static void _time(Time start, Timer that)
> {
> const Time stop = Clock::now();
>
> double value;
>
> process::internal::acquire(&that.data->lock);
> {
> that.data->lastValue = T(stop - start).value();
> value = that.data->lastValue.get();
> }
> process::internal::release(&that.data->lock);
>
> that.push(value);
> }
> ```
>
>
> Diffs
> -
>
> docs/mesos-c++-style-guide.md 439fe12
>
> Diff: https://reviews.apache.org/r/32536/diff/
>
>
> Testing
> ---
>
> None (not a functional change).
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32536: Updated variable naming style.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32536/
---
(Updated April 14, 2015, 1:08 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Michael Park, Niklas
Nielsen, and Timothy Chen.
Changes
---
added JIRA
Bugs: MESOS-2616
https://issues.apache.org/jira/browse/MESOS-2616
Repository: mesos-incubating
Description
---
Documents the patterns we use to name variables and function arguments in our
codebase.
Leading underscores to avoid ambiguity.
===
We use this pattern extensively in libprocess, stout and mesos, a few examples
below.
* `stout/try.hpp:105`
```
Try(State _state, T* _t = NULL, const std::string& _message = "")
: state(_state), t(_t), message(_message) {}
```
* `process/http.hpp:480`
```
URL(const std::string& _scheme,
const std::string& _domain,
const uint16_t _port = 80,
const std::string& _path = "/",
const hashmap& _query =
(hashmap()),
const Option& _fragment = None())
: scheme(_scheme),
domain(_domain),
port(_port),
path(_path),
query(_query),
fragment(_fragment) {}
```
* `slave/containerizer/linux_launcher.cpp:56`
```
LinuxLauncher::LinuxLauncher(
const Flags& _flags,
int _namespaces,
const string& _hierarchy)
: flags(_flags),
namespaces(_namespaces),
hierarchy(_hierarchy) {}
```
Trailing undescores as prime symbols.
=
We use this pattern in the code, though not extensively. I would like to see
more pass-by-value instead of creating copies from a variable passed by const
reference.
* `master.cpp:2942`
```
// Create and add the slave id.
SlaveInfo slaveInfo_ = slaveInfo;
slaveInfo_.mutable_id()->CopyFrom(newSlaveId());
```
* `slave.cpp:4180`
```
ExecutorInfo executorInfo_ = executor->info;
Resources resources = executorInfo_.resources();
resources += taskInfo.resources();
executorInfo_.mutable_resources()->CopyFrom(resources);
```
* `status_update_manager.cpp:474`
```
// Bounded exponential backoff.
Duration duration_ =
std::min(duration * 2, STATUS_UPDATE_RETRY_INTERVAL_MAX);
```
* `containerizer/mesos/containerizer.cpp:109`
```
// Modify the flags to include any changes to isolation.
Flags flags_ = flags;
flags_.isolation = isolation;
```
Passing arguments by value.
===
* `slave.cpp:2480`
```
void Slave::statusUpdate(StatusUpdate update, const UPID& pid)
{
...
// Set the source before forwarding the status update.
update.mutable_status()->set_source(
pid == UPID() ? TaskStatus::SOURCE_SLAVE : TaskStatus::SOURCE_EXECUTOR);
...
}
```
* `process/metrics/timer.hpp:103`
```
static void _time(Time start, Timer that)
{
const Time stop = Clock::now();
double value;
process::internal::acquire(&that.data->lock);
{
that.data->lastValue = T(stop - start).value();
value = that.data->lastValue.get();
}
process::internal::release(&that.data->lock);
that.push(value);
}
```
Diffs
-
docs/mesos-c++-style-guide.md 439fe12
Diff: https://reviews.apache.org/r/32536/diff/
Testing
---
None (not a functional change).
Thanks,
Alexander Rukletsov
Re: Review Request 32536: Updated variable naming style.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32536/#review77916
---
Patch looks great!
Reviews applied: [32536]
All tests passed.
- Mesos ReviewBot
On March 26, 2015, 4:10 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32536/
> ---
>
> (Updated March 26, 2015, 4:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Michael Park, Niklas
> Nielsen, and Timothy Chen.
>
>
> Repository: mesos-incubating
>
>
> Description
> ---
>
> Documents the patterns we use to name variables and function arguments in our
> codebase.
>
> Leading underscores to avoid ambiguity.
> ===
>
> We use this pattern extensively in libprocess, stout and mesos, a few
> examples below.
>
> * `stout/try.hpp:105`
> ```
> Try(State _state, T* _t = NULL, const std::string& _message = "")
> : state(_state), t(_t), message(_message) {}
> ```
>
> * `process/http.hpp:480`
> ```
> URL(const std::string& _scheme,
> const std::string& _domain,
> const uint16_t _port = 80,
> const std::string& _path = "/",
> const hashmap& _query =
> (hashmap()),
> const Option& _fragment = None())
> : scheme(_scheme),
> domain(_domain),
> port(_port),
> path(_path),
> query(_query),
> fragment(_fragment) {}
> ```
>
> * `slave/containerizer/linux_launcher.cpp:56`
> ```
> LinuxLauncher::LinuxLauncher(
> const Flags& _flags,
> int _namespaces,
> const string& _hierarchy)
> : flags(_flags),
> namespaces(_namespaces),
> hierarchy(_hierarchy) {}
> ```
>
> Trailing undescores as prime symbols.
> =
>
> We use this pattern in the code, though not extensively. I would like to see
> more pass-by-value instead of creating copies from a variable passed by const
> reference.
>
> * `master.cpp:2942`
> ```
> // Create and add the slave id.
> SlaveInfo slaveInfo_ = slaveInfo;
> slaveInfo_.mutable_id()->CopyFrom(newSlaveId());
> ```
>
> * `slave.cpp:4180`
> ```
> ExecutorInfo executorInfo_ = executor->info;
> Resources resources = executorInfo_.resources();
> resources += taskInfo.resources();
> executorInfo_.mutable_resources()->CopyFrom(resources);
> ```
>
> * `status_update_manager.cpp:474`
> ```
> // Bounded exponential backoff.
> Duration duration_ =
> std::min(duration * 2, STATUS_UPDATE_RETRY_INTERVAL_MAX);
> ```
>
> * `containerizer/mesos/containerizer.cpp:109`
> ```
> // Modify the flags to include any changes to isolation.
> Flags flags_ = flags;
> flags_.isolation = isolation;
> ```
>
> Passing arguments by value.
> ===
>
> * `slave.cpp:2480`
> ```
> void Slave::statusUpdate(StatusUpdate update, const UPID& pid)
> {
> ...
> // Set the source before forwarding the status update.
> update.mutable_status()->set_source(
> pid == UPID() ? TaskStatus::SOURCE_SLAVE : TaskStatus::SOURCE_EXECUTOR);
> ...
> }
> ```
>
> * `process/metrics/timer.hpp:103`
> ```
> static void _time(Time start, Timer that)
> {
> const Time stop = Clock::now();
>
> double value;
>
> process::internal::acquire(&that.data->lock);
> {
> that.data->lastValue = T(stop - start).value();
> value = that.data->lastValue.get();
> }
> process::internal::release(&that.data->lock);
>
> that.push(value);
> }
> ```
>
>
> Diffs
> -
>
> docs/mesos-c++-style-guide.md 439fe12
>
> Diff: https://reviews.apache.org/r/32536/diff/
>
>
> Testing
> ---
>
> None (not a functional change).
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Review Request 32536: Updated variable naming style.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32536/
---
Review request for mesos, Benjamin Hindman, Ben Mahler, Michael Park, Niklas
Nielsen, and Timothy Chen.
Repository: mesos-incubating
Description
---
Documents the patterns we use to name variables and function arguments in our
codebase.
Leading underscores to avoid ambiguity.
===
We use this pattern extensively in libprocess, stout and mesos, a few examples
below.
* `stout/try.hpp:105`
```
Try(State _state, T* _t = NULL, const std::string& _message = "")
: state(_state), t(_t), message(_message) {}
```
* `process/http.hpp:480`
```
URL(const std::string& _scheme,
const std::string& _domain,
const uint16_t _port = 80,
const std::string& _path = "/",
const hashmap& _query =
(hashmap()),
const Option& _fragment = None())
: scheme(_scheme),
domain(_domain),
port(_port),
path(_path),
query(_query),
fragment(_fragment) {}
```
* `slave/containerizer/linux_launcher.cpp:56`
```
LinuxLauncher::LinuxLauncher(
const Flags& _flags,
int _namespaces,
const string& _hierarchy)
: flags(_flags),
namespaces(_namespaces),
hierarchy(_hierarchy) {}
```
Trailing undescores as prime symbols.
=
We use this pattern in the code, though not extensively. I would like to see
more pass-by-value instead of creating copies from a variable passed by const
reference.
* `master.cpp:2942`
```
// Create and add the slave id.
SlaveInfo slaveInfo_ = slaveInfo;
slaveInfo_.mutable_id()->CopyFrom(newSlaveId());
```
* `slave.cpp:4180`
```
ExecutorInfo executorInfo_ = executor->info;
Resources resources = executorInfo_.resources();
resources += taskInfo.resources();
executorInfo_.mutable_resources()->CopyFrom(resources);
```
* `status_update_manager.cpp:474`
```
// Bounded exponential backoff.
Duration duration_ =
std::min(duration * 2, STATUS_UPDATE_RETRY_INTERVAL_MAX);
```
* `containerizer/mesos/containerizer.cpp:109`
```
// Modify the flags to include any changes to isolation.
Flags flags_ = flags;
flags_.isolation = isolation;
```
Passing arguments by value.
===
* `slave.cpp:2480`
```
void Slave::statusUpdate(StatusUpdate update, const UPID& pid)
{
...
// Set the source before forwarding the status update.
update.mutable_status()->set_source(
pid == UPID() ? TaskStatus::SOURCE_SLAVE : TaskStatus::SOURCE_EXECUTOR);
...
}
```
* `process/metrics/timer.hpp:103`
```
static void _time(Time start, Timer that)
{
const Time stop = Clock::now();
double value;
process::internal::acquire(&that.data->lock);
{
that.data->lastValue = T(stop - start).value();
value = that.data->lastValue.get();
}
process::internal::release(&that.data->lock);
that.push(value);
}
```
Diffs
-
docs/mesos-c++-style-guide.md 439fe12
Diff: https://reviews.apache.org/r/32536/diff/
Testing
---
None (not a functional change).
Thanks,
Alexander Rukletsov
