Competent developers make mistakes too. I think it’s worth at least skimming every line in a PR. Things like indexing out of bounds should probably be handled by testing or using a memory safe language.
I don’t think code review is the place where you should be trying to catch bugs in the minutia of the code. You should be assessing readability/maintainability, architectural decisions, risk, and testing strategy. That does involve reading all of the code, even if only by skimming.
I guess I don’t really count skimming as reading haha
When looking for readability, maintainability, etc what I mainly look for are design patterns. If you’re using the same boilerplate and layout I’m going to start skipping over sections
I also mainly work on high performance embedded systems that are difficult/nearly impossible to unit test and I dont always have line coverage metrics. A lot more verification by inspection in my corner of the world
There’s a couple big reasons, sometimes you only have to deal with one or two, other times you’re hit with all of them.
The data source for the algorithms often comes from arbitrary memory addresses that live outside the context of the C, C++, or Rust runtime. If I want to read CPU core temp I send magic numbers to one address to configure ADC channel conversion, another magic number somewhere else to do the conversion, wait a few dozen microseconds, then read a third new address that I squint one eye, spin around 10 times, and do a handstand and I’ll get a number that looks like temperature.
If I’m running on the microcontroller, I’m essentially in production environment and I can’t control what the data is. If I recompile for a different system (say x86 Linux) then I’m almost certainly going to segfault. I could abstract out the reading of the data, but then on my tiny 8MHz microcontroller that has to run it’s main loop at 1000Hz, I only have a few thousand clock cycles to get everything done, calling functions, pushing/popping the stack takes a lot of time.
Adding to the pile-on most microcontrollers have <1MB of RAM, many in the tens of kilobytes range so I don’t have much of a stack to work with and I need to keep even the number of instructions down.
Even if you do all the abstractions, you then have extra logic you have to write to simulate what is happening and subtle behaviors that are timing dependant. It frequently is extremely difficult to do that as the documentation rarely fully describes everything, so you can lull yourself into a false sense of security that you’ve tested the code and it’s correct, but then you get stuck on a register value that never changes and a watchdog timer comes along and resets the hardware on you.
And that’s not the getting into the runtime or build environments, flashing procedures, limited flash cycles, and a million other business constraints that technically you can do it, but at that point your dev team has all taken the month off to go play Half Life 3
That definitely increases the challenge of writing tests or trusting that they are faithfully representing your production environment.
I could abstract out the reading of the data
I was going to suggest this, and it sounds like that ends up being expensive. It seems like you need some kind of test harness that’s able to emulate your specific hardware while still allowing for testing hooks that don’t impede the tight clock scheduling.
Competent developers make mistakes too. I think it’s worth at least skimming every line in a PR. Things like indexing out of bounds should probably be handled by testing or using a memory safe language.
I don’t think code review is the place where you should be trying to catch bugs in the minutia of the code. You should be assessing readability/maintainability, architectural decisions, risk, and testing strategy. That does involve reading all of the code, even if only by skimming.
I guess I don’t really count skimming as reading haha
When looking for readability, maintainability, etc what I mainly look for are design patterns. If you’re using the same boilerplate and layout I’m going to start skipping over sections
I also mainly work on high performance embedded systems that are difficult/nearly impossible to unit test and I dont always have line coverage metrics. A lot more verification by inspection in my corner of the world
Sort of a change in topic, but I’m curious why that would be so difficult to test.
There’s a couple big reasons, sometimes you only have to deal with one or two, other times you’re hit with all of them.
The data source for the algorithms often comes from arbitrary memory addresses that live outside the context of the C, C++, or Rust runtime. If I want to read CPU core temp I send magic numbers to one address to configure ADC channel conversion, another magic number somewhere else to do the conversion, wait a few dozen microseconds, then read a third new address that I squint one eye, spin around 10 times, and do a handstand and I’ll get a number that looks like temperature.
If I’m running on the microcontroller, I’m essentially in production environment and I can’t control what the data is. If I recompile for a different system (say x86 Linux) then I’m almost certainly going to segfault. I could abstract out the reading of the data, but then on my tiny 8MHz microcontroller that has to run it’s main loop at 1000Hz, I only have a few thousand clock cycles to get everything done, calling functions, pushing/popping the stack takes a lot of time.
Adding to the pile-on most microcontrollers have <1MB of RAM, many in the tens of kilobytes range so I don’t have much of a stack to work with and I need to keep even the number of instructions down.
Even if you do all the abstractions, you then have extra logic you have to write to simulate what is happening and subtle behaviors that are timing dependant. It frequently is extremely difficult to do that as the documentation rarely fully describes everything, so you can lull yourself into a false sense of security that you’ve tested the code and it’s correct, but then you get stuck on a register value that never changes and a watchdog timer comes along and resets the hardware on you.
And that’s not the getting into the runtime or build environments, flashing procedures, limited flash cycles, and a million other business constraints that technically you can do it, but at that point your dev team has all taken the month off to go play Half Life 3
That definitely increases the challenge of writing tests or trusting that they are faithfully representing your production environment.
I was going to suggest this, and it sounds like that ends up being expensive. It seems like you need some kind of test harness that’s able to emulate your specific hardware while still allowing for testing hooks that don’t impede the tight clock scheduling.