datafuselabs / databend

๐——๐—ฎ๐˜๐—ฎ, ๐—”๐—ป๐—ฎ๐—น๐˜†๐˜๐—ถ๐—ฐ๐˜€ & ๐—”๐—œ. Modern alternative to Snowflake. Cost-effective and simple for massive-scale analytics. https://databend.com
https://docs.databend.com
Other
7.29k stars 701 forks source link

fix: solve some inconsistencies between code and comments #15394

Closed YichiZhang0613 closed 2 weeks ago

YichiZhang0613 commented 2 weeks ago

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Solve the inconsistencies between code and comments which are proposed in #15390

Tests

Type of change


This change isโ€‚Reviewable

Dousir9 commented 2 weeks ago

I think assert in this PR is unnecessary as Rust automatically checks bounds on array accesses (and panic's if it is out of bounds).

YichiZhang0613 commented 2 weeks ago

I think assert in this PR is unnecessary as Rust automatically checks bounds on array accesses (and panic's if it is out of bounds). In my opinion, the Rust official documentation recommends that to prevent array index out-of-bounds errors, it is recommended to use methods such as assert! for verification, or use built-in methods such as get and insert instead of directly accessing elements. This approach can handle potential errors more safely. In addition, conscious processing can prompt meaningful information during panic, thus achieving better user interaction. What's more, many other methods in databend also have assert! checks for out-of-bounds situations, which can also better maintain consistency of whole project.

Dousir9 commented 2 weeks ago

I think assert in this PR is unnecessary as Rust automatically checks bounds on array accesses (and panic's if it is out of bounds). In my opinion, the Rust official documentation recommends that to prevent array index out-of-bounds errors, it is recommended to use methods such as assert! for verification, or use built-in methods such as get and insert instead of directly accessing elements. This approach can handle potential errors more safely. In addition, conscious processing can prompt meaningful information during panic, thus achieving better user interaction. What's more, many other methods in databend also have assert! checks for out-of-bounds situations, which can also better maintain consistency of whole project.

For a simple data structure like Vec, we don't use get to access it, Rust automatically checks bounds on array accesses, so assert is unnecessary, we can add some comments for it. In addition, assert is not free, it will affect performance.

YichiZhang0613 commented 2 weeks ago

I think assert in this PR is unnecessary as Rust automatically checks bounds on array accesses (and panic's if it is out of bounds). In my opinion, the Rust official documentation recommends that to prevent array index out-of-bounds errors, it is recommended to use methods such as assert! for verification, or use built-in methods such as get and insert instead of directly accessing elements. This approach can handle potential errors more safely. In addition, conscious processing can prompt meaningful information during panic, thus achieving better user interaction. What's more, many other methods in databend also have assert! checks for out-of-bounds situations, which can also better maintain consistency of whole project.

For a simple data structure like Vec, we don't use get to access it, Rust automatically checks bounds on array accesses, so assert is unnecessary, we can add some comments for it. In addition, assert is not free, it will affect performance.

OK, I accept your point and modified my PR accordingly.