You do that inherently when you are writing the code. Every line you write has already had that focus, because you had to come up with it.
That you should also not trust every line of the compiled assembly after you wrote the code, and check it lime by line, is an extremely stupid argument.
Of course, you should get someone else, with a fresh/unbiased opinion to review your code too, but just the act of figuring out every line you write is in some sense a rewiev, that there is at least a plan or reason why it was written.
AI just randomly generates code without thought or reason, and you have to check everything. Unlike assembly generated from code.
That you should also not trust every line of the compiled assembly after you wrote the code, and check it lime by line, is an extremely stupid argument.
I guess it’s good that no one is making that argument then.
that is the argument linus is making… he is comparing a deterministic tool (a compiler that outputs the same assembly every time) to a non-deterministic tool and saying that they are the same type of tool. that require the same level of review, when they do not.
he is comparing a deterministic tool with a non-deterministic tool
Yes, but that’s not the argument Mikina was stating. Linus was never advocating for reading assembly line by line. Linus was using that as a satirical example, not suggesting anyone should actually do that.
Not parent commenter, but no, not to the same degree.
The level of trust I have in a source greatly changes how I review it. If I know that I’m reviewing code from a competent developer, I’m going to see a for loop and look for why is this here, how does it play into the larger objective and if I’m happy with it, I move on. But if I don’t trust the developer as much, I’m looking at far more like what data type is being used in the indexing, what’s the break condition, can go off the rails into random memory.
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.
If unit tests are being done, some kind of emulator is usually what gets used and the scope of testing is significantly limited.
The more common approach is to abandon unit tests all together and go for board functional tests where as many of the external interfaces are controlled and put through representative loads. The complexity of the software is rarely that high so we can be pretty confident that we’re covering everything we need to care about anyways.
Do you not think all code deserves this level of focus?
You do that inherently when you are writing the code. Every line you write has already had that focus, because you had to come up with it.
That you should also not trust every line of the compiled assembly after you wrote the code, and check it lime by line, is an extremely stupid argument.
Of course, you should get someone else, with a fresh/unbiased opinion to review your code too, but just the act of figuring out every line you write is in some sense a rewiev, that there is at least a plan or reason why it was written.
AI just randomly generates code without thought or reason, and you have to check everything. Unlike assembly generated from code.
I guess it’s good that no one is making that argument then.
that is the argument linus is making… he is comparing a deterministic tool (a compiler that outputs the same assembly every time) to a non-deterministic tool and saying that they are the same type of tool. that require the same level of review, when they do not.
Yes, but that’s not the argument Mikina was stating. Linus was never advocating for reading assembly line by line. Linus was using that as a satirical example, not suggesting anyone should actually do that.
Not parent commenter, but no, not to the same degree.
The level of trust I have in a source greatly changes how I review it. If I know that I’m reviewing code from a competent developer, I’m going to see a for loop and look for why is this here, how does it play into the larger objective and if I’m happy with it, I move on. But if I don’t trust the developer as much, I’m looking at far more like what data type is being used in the indexing, what’s the break condition, can go off the rails into random memory.
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.
If unit tests are being done, some kind of emulator is usually what gets used and the scope of testing is significantly limited.
The more common approach is to abandon unit tests all together and go for board functional tests where as many of the external interfaces are controlled and put through representative loads. The complexity of the software is rarely that high so we can be pretty confident that we’re covering everything we need to care about anyways.