3 ms·

It doesn't change the point I'm making that just because something is written in Rust means it will require less strictness of code review. There might be a cl

It doesn't change the point I'm making that just because something is written in Rust means it will require less strictness of code review. There might be a class of bugs that are more difficult to hit in rust but that's just one of many bug types. You can still write terribly buggy code in rust. Assuming less strict review is necessary would be overall worse for the project and likely let _more_ bugs in.

mustache_kimono 4y ago
> Assuming less strict review is necessary would be overall worse for the project. Maybe? It might make it easier for those less experienced with C/C++ memory safety issues to review? Instead of thinking of it as being less strict, I might think of it as -- freeing the reviewer up to focus on other issues.
Ar-Curunir 4y ago
It means that you can focus your attention on catching logic bugs, instead of dividing attention between memory safety and logic bugs. That is surely a win, no?
wtetzner 4y ago
I think it will make the code reviews potentially more interesting, as it removes a class of things that needed to be dealt with before.
tialaramex 4y ago
Let's make it a bit more concrete with an actual example of submitted code. Here's a recent patch I wrote "Improve E0308: suggest user meant to use byte literal, w/ tests and fix" which adds a suggestion for Rust's diagnostic when you write for example '*' the literal char, Unicode U+002A but it needed a single byte and that's not what a char is. My code suggests adding the prefix b, so writing b'*' here, meaning the ASCII code for that symbol 0x2A which is a single byte :: https://github.com/tialaramex/rust/commit/130d02b62e65c5f2a434eaec63c4249e9d508487 https://github.com/tialaramex/rust/commit/130d02b62e65c5f2a4... I wrote that code but I don't understand the internals of how self.tcx.sess().source_map().span_to_snippet(span) works. Not my problem, we're in the diagnostics code so even if this is perhaps slightly slower than optimal it doesn't matter because a human will need to read this output and act on it - E0308 is a type mismatch, the program does not compile as written. Does my reviewer know? Maybe, I didn't ask them, but they don't really need to, it's clearly fine here to call this stuff, there won't be a nasty surprise "Oh, make sure you restore the FQ5 when setting Z due to calling sess() in this code" because that's not how Rust works whereas in a language like C++ of course such traps may exist. Now there is some risk I made a logic mistake, but, I wrote tests for this of course, unlike with subtle memory safety bugs, logic bugs are often caught by proper testing. My tests here are somewhat superficial, I check 'X' and '#' which should both cause the suggestion, and I check '€' which should not, but I think they cover the cases the compiler will really see here.

mustache_kimono 4y ago

> Assuming less strict review is necessary would be overall worse for the project. Maybe? It might make it easier for those less experienced with C/C++ memory safety issues to review? Instead of thinking of it as being less strict, I might think of it as -- freeing the reviewer up to focus on other issues.

Ar-Curunir 4y ago

It means that you can focus your attention on catching logic bugs, instead of dividing attention between memory safety and logic bugs. That is surely a win, no?

wtetzner 4y ago

I think it will make the code reviews potentially more interesting, as it removes a class of things that needed to be dealt with before.

tialaramex 4y ago

Let's make it a bit more concrete with an actual example of submitted code. Here's a recent patch I wrote "Improve E0308: suggest user meant to use byte literal, w/ tests and fix" which adds a suggestion for Rust's diagnostic when you write for example '*' the literal char, Unicode U+002A but it needed a single byte and that's not what a char is. My code suggests adding the prefix b, so writing b'*' here, meaning the ASCII code for that symbol 0x2A which is a single byte :: https://github.com/tialaramex/rust/commit/130d02b62e65c5f2a434eaec63c4249e9d508487 https://github.com/tialaramex/rust/commit/130d02b62e65c5f2a4... I wrote that code but I don't understand the internals of how self.tcx.sess().source_map().span_to_snippet(span) works. Not my problem, we're in the diagnostics code so even if this is perhaps slightly slower than optimal it doesn't matter because a human will need to read this output and act on it - E0308 is a type mismatch, the program does not compile as written. Does my reviewer know? Maybe, I didn't ask them, but they don't really need to, it's clearly fine here to call this stuff, there won't be a nasty surprise "Oh, make sure you restore the FQ5 when setting Z due to calling sess() in this code" because that's not how Rust works whereas in a language like C++ of course such traps may exist. Now there is some risk I made a logic mistake, but, I wrote tests for this of course, unlike with subtle memory safety bugs, logic bugs are often caught by proper testing. My tests here are somewhat superficial, I check 'X' and '#' which should both cause the suggestion, and I check '€' which should not, but I think they cover the cases the compiler will really see here.