DioxusLabs / DioxusLabs/taffy

Add comment about comparison with f32::INFINITY

Open
#492 1 comment 0 reactions 0 assignees View on GitHub
code quality
Dominant language
Rust
Stars
3.6k
Forks
222
Avg merge
10h 41m
Merged PRs (30d)
40

Description

During reading sources in https://github.com/DioxusLabs/taffy/blob/main/src/compute/grid/track_sizing.rs I found a lot of direct checks for `f32::INFINITY`.

This is the place where this comparison is used most often . We can easily check it:
```shell
➜ src grep -rn f32::INFINITY
style/dimension.rs:271: AvailableSpace::MaxContent => f32::INFINITY,
compute/grid/types/grid_track.rs:134: None => f32::INFINITY,
compute/grid/types/grid_track.rs:137: _ => f32::INFINITY,
compute/grid/track_sizing.rs:374: track.growth_limit = if track.growth_limit == f32::INFINITY {
compute/grid/track_sizing.rs:417: track.max_track_sizing_function.definite_value(axis_inner_node_size).unwrap_or(f32::INFINITY);
compute/grid/track_sizing.rs:626: track.growth_limit = if track.growth_limit == f32::INFINITY {
compute/grid/track_sizing.rs:689: |_| f32::INFINITY,
compute/grid/track_sizing.rs:726: |_| f32::INFINITY,
compute/grid/track_sizing.rs:787: |_| f32::INFINITY,
compute/grid/track_sizing.rs:823: |_| f32::INFINITY,
compute/grid/track_sizing.rs:890: .filter(|track| track.growth_limit == f32::INFINITY)
compute/grid/track_sizing.rs:1049: .map(|track| if track.growth_limit == f32::INFINITY { track.base_size } else { track.growth_limit })
compute/grid/track_sizing.rs:1061: track.infinitely_growable || track.fit_content_limited_growth_limit(axis_inner_node_size) == f32::INFINITY
compute/grid/track_sizing.rs:1067: track.infinitely_growable || track.fit_content_limited_growth_limit(axis_inner_node_size) == f32::INFINITY
compute/grid/track_sizing.rs:1080: |track| if track.growth_limit == f32::INFINITY { track.base_size } else { track.growth_limit },
compute/grid/track_sizing.rs:1107: if free_space == f32::INFINITY {
compute/grid/track_sizing.rs:1206: let axis_max_size = axis_max_size.unwrap_or(f32::INFINITY);
compute/grid/track_sizing.rs:1241: let mut hypothetical_fr_size = f32::INFINITY;
compute/flexbox.rs:931: style_max.maybe_min(flex_basis_max).or(flex_basis_max).unwrap_or(f32::INFINITY);
```

I found this "unusual", as it is generally considered a "safe" check for `inf` via [`f32::is_infinite()`](https://doc.rust-lang.org/std/primitive.f32.html#method.is_infinite) or [`isinf()`](https://en.cppreference.com/w/cpp/numeric/math/isinf) in C++ or C. This pattern is similar to: "using `is_nan()` better then direct comparation with NaN" - that's why it caught my attention here.

After more detailed consideration of what is happening in the code, I came to the conclusion that a current direct comparison with `f32::INFINTIY` is still a better option (over `f32::is_infinite()`) - it does not take into account negative infinity, because in such places checks are performed for a special "marker" or "flag", which is only **positive infinity**, so these are checks NOT for `inf` produced by division-by-zero somewhere.

@nicoburns I can not comprehend what is happening in that file - as the owner of sacred knowledge, can you add a descriptive comment (for example: at the beginning of the file, somewhere after `mod` imports, before functions declaration)? - With description like:

```rust
/* NOTE

Here, in some places used direct comparation with (positive-only) `f32::INFINITY`,
instead of checking it with `f32::is_infinite()` (that also takes negative infinity
into account).

For example:

free_space == f32::INFINITY

istead of:

free_space.is_infinite() // same as isinf(free_space) in C

It's normal, dont change it, since in such cases we check only for special
marker (`positive inf`) , returned by
callbacks for example, and no need to check it via `f32::is_infinite()`, since
`negative inf` not expected here.
*/
```

so after reading it no one tries to "fix it", and it does not attract attention :)

Contributor guide

Open the contributing guide

Research direction

Read src/compute/grid/track_sizing.rs and run grep -rn f32::INFINITY from src to review the comparison sites. Add a concise comment explaining why positive-only f32::INFINITY comparisons are intentional instead of f32::is_infinite(). Done means the rationale is documented near the file's imports or function declarations.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
documentation
Issue type
Documentation
Difficulty
1/5
Estimated time
Under an hour
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.