CL? Is that perforce? Hopefully it works better than the real perforce. A really tough and confusing versioning tool.
think "PR"
How to do a code review
71–80 of 376 posts
Re: How to do a code review
#72I feel embarrassed to even ask... what does CL stand for?
Changelist, it's perforce for "patch"
Re: How to do a code review
#73I'd add two things, from a decade of experience at Google: Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be c…
> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…
I'm not saying you should write code in a performance-oblivious fashion. I am saying that if you want to do something non-canonical, or you want an exception to style guide or other established guidelines, or if you write a comment in your code that says "This function is faster than std::foo", then you must have evidence. In the absence of evidence, adhere to normal rules.
I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". It had been written by a very famous engineer, and it was even true in the days of the Pentium III processor. But at the time I found it it was not only false but the code as written was slower on modern CPUs than the shorter and totally obvious equivalent.
Re: How to do a code review
#74In the "What Is Not An Emergency?" section: > It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend. I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend. Who shares the same experience?
We call it a chuck-in.
Re: How to do a code review
#75CL? Is that perforce? Hopefully it works better than the real perforce. A really tough and confusing versioning tool.
> CL: Stands for “changelist,” which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change” or a “patch.” source: https://google.github.io/eng-practices/
Yes, I understand, I used it every day. Just very surprised google used perforce, given my bad experiences with it as a whole.
Re: How to do a code review
#76In the "What Is Not An Emergency?" section: > It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend. I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend. Who shares the same experience?
They didn't give the two examples that are most common inside Google: Google I/O is coming up, or annual performance reviews are coming up.
Strangely they also list as "not an emergency" rollbacks of clearly broken code, but that's an exception to review rules inside Google. Anyone can do a pure rollback of a change without getting the approval of the owners of the code, and there are automated tools that will roll back changes that appear to have broken at least 1000 tests, without human review.
Edit: There's even a button right in the code review web UI to roll back a committed change.
Re: How to do a code review
#77Am I the only software engineer in the world who hates diff-based code review? Imagine trying to evaluate the quality of a novel by examining diffs to the manuscript. Reviewing every diff means that every single change needs to yield a good novel. But that's an absurd constraint on the creative process - what if you want to introduce a new important character? You check in a new version of the first chapter where the…
I am a software engineer and an amateur writer/novelist.
I am the only person looking at my manuscripts before I finish a second draft. There is no risk that my half-finished work is going to introduce cruft into a shared "codebase" and/or "bugs" into "prod". Either they make it into that second draft state and receive feedback as whole, finished things, or they die inside my computer.
This is very different from typical software development as part of a team, and your teammates should recognize incremental green-field work and adjust their review criteria accordingly. It's still important to review this sort of code if it goes anywhere near production systems, though, for obvious reasons.
Ironically, since I write in Vim and auto-wrap with par, it's harder for me to generate nice diffs than it would be if I just wrote in Google Docs like a good citizen of the future. When it comes to interacting with editors and other collaborators, though, it's super important to collect and review "diffs" of some form. Horror stories abound of unexpected changes making it to print.
Re: How to do a code review
#78I'd add two things, from a decade of experience at Google: Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be c…
> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…
I agree - most folks overestimate the effectiveness of microbenchmarks. I work in a field where microbenchmarks often feel useless: computer graphics. Often we have to consider the entire pipeline of work. Writing to a texture in Stage 3 may seem perfectly fine, but it could be thrashing the cache in Stage 4 when the texture is being read from. Benchmarking them separately misses this.
Re: How to do a code review
#79I'd add two things, from a decade of experience at Google: Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be c…
> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…
You have to be careful though. The compiler can do a very good job of lifting statements (and many other optimizations), so it is generally a better use of developer time to try and write code that is clear than to write code that is believed (without evidence) to be more performant.
Re: How to do a code review
#80> Some hardward manufacturers only ship new hardware once a year.
A line snuck in through their code review.
One big issue that I'm not really seeing discussed is that in many cases code reviews end up being very subjective. "hard to maintain", "not readable", "brittle", "needs more tests" are all super subjective. There is no magic tool that can tell you the future maintenance costs (vs. the current costs), whether some area is more likely to see issues, etc. A lot of this is a matter of judgement. One old recommendation I recall is to limit the scope of code reviews to finding bugs. If there is a bug that's pretty objective, the code does not work.