//net code reviews doc

[Adam Rice]

Looks like we never announced this, but there's now a guide for code reviews for //net:

https://source.chromium.org/chromium/chromium/src/+/main:net/docs/code-reviews.md

I suggest reading through it once, then using it as a checklist.

[Yoichi Osato]

Looks good!

Some questions:

> `std::vector<uint8_t>` should be used for binary data.

> A large amount of legacy code in //net uses `std::string` for binary data.

Any code-clean effort to make this better?

I am also interested in how many `std::string` copy constructors and `std::vector::push_back()` without `reserve()` are used in //net.

2025年1月29日(水) 13:16 Adam Rice <ri...@chromium.org>:

Looks like we never announced this, but there's now a guide for code reviews for //net:

https://source.chromium.org/chromium/chromium/src/+/main:net/docs/code-reviews.md

I suggest reading through it once, then using it as a checklist.

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAC_ixdyW4FB%2BhdwcFKg%3D476Q-%2B_j%2BOb6d02xyUu35D6rHRvTWg%40mail.gmail.com.

[Adam Rice]

> A large amount of legacy code in //net uses `std::string` for binary data.
Any code-clean effort to make this better?

There's some work ongoing in https://issues.chromium.org/issues/40855435 to clean up the DNS code, which is a very prolific offender.

I am also interested in how many `std::string` copy constructors and `std::vector::push_back()` without `reserve()` are used in //net.

std::string copy constructors: I tried to count these using code search, but its facility for restricting references to those matching a path doesn't have a way to anchor at the start of the path. So, the answer is roughly 759 including some other things that happen to have "net/" in the path and excluding tests.

Counting push_backs that don't follow a reserve() is rather more work than I have time for, but I can count calls:

• std::vector::push_back: roughly 260 for const&, 1592 for &&, total 1852
• std::vector::reserve: 100

As above, this randomly includes other stuff that happens to have "net/" in the path.

Of course, it's not always wrong to use push_back() without reserve(). If you don't know what the final size will be, but it's usually 0 or 1 anyway, then there's no point in calling reserve().

I'm a big fan of base::ToVector() because it does the right thing automatically. You can rewrite

std::vector<std::string> cert_bytes;
for (const auto& cert : certs) {
  cert_bytes.push_back(cert->der_cert().AsString());
}

as

std::vector<std::string> cert_bytes = base::ToVector(
    certs,
    [](const auto& cert) { return cert->der_cert().AsString(); });

and it reserves the right size for you.

[Yoichi Osato]

Got it. I didn't know about the `base::ToVector` feature.

Thanks!

2025年1月31日(金) 18:36 Adam Rice <ri...@chromium.org>: