google / google/differential-privacy
Improving comments in privacy-critical code blocks.
- Dominant language
- Go
- Stars
- 3.4k
- Forks
- 434
- Avg merge
- 4d 9h
- Merged PRs (30d)
- 9
Description
The lack of comments makes this code harder to audit. Consider https://github.com/google/differential-privacy/blob/master/differential_privacy/algorithms/rand.cc
It is not clear from reading the code what `SecureURBG::RefreshCache` does. Is it that you are computing a whole buffer of randomness at once and then feeding it out byte-by-byte? That is not a cache, then, but a buffer.
In `SecureURBG::result_type SecureURBG::operator`, it is not clear why `old_index` is copied from `current_index` before the `memcpy` operation:
```
SecureURBG::result_type SecureURBG::operator()() {
absl::WriterMutexLock lock(&mutex_);
if (current_index_ + sizeof(result_type) > kCacheSize) {
RefreshCache();
}
int old_index = current_index_;
current_index_ += sizeof(result_type);
result_type result;
std::memcpy(&result, cache_ + old_index, sizeof(result_type));
return result;
}
```
Why the extra copy, and not simply copy it like this:
```
SecureURBG::result_type SecureURBG::operator()() {
absl::WriterMutexLock lock(&mutex_);
if (current_index_ + sizeof(result_type) > kCacheSize) {
RefreshCache();
}
result_type result;
std::memcpy(&result, cache_ + current_index_, sizeof(result_type));
current_index_ += sizeof(result_type);
return result;
}
```
The programmer's intent is not clear from the C++ code, so it would be useful to have comments to explain it. It would also be nice to know what URBG stands for.
Contributor guide
Assessment
This issue has not been assessed yet.