Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add comment about comparison with f32::INFINITY #492

Open
inobelar opened this issue Jun 2, 2023 · 1 comment
Open

Add comment about comparison with f32::INFINITY #492

inobelar opened this issue Jun 2, 2023 · 1 comment
Labels
code quality Make the code cleaner or prettier.

Comments

@inobelar
Copy link
Contributor

inobelar commented Jun 2, 2023

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:

➜  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() or 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:

/* 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`) <ADD DESCRIPTION FOR IT HERE>, 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 :)

@alice-i-cecile alice-i-cecile added the code quality Make the code cleaner or prettier. label Jun 3, 2023
@nicoburns
Copy link
Collaborator

nicoburns commented Jun 3, 2023

Seems like a good idea to add a comment. I'll turn this into a proper comment when I get chance, but the short answer to why only positive infinity is that it's being used as a value for the "growth limit" for track (row/column) sizes. These start at zero, and the algorithm progressively increases their size up to (and sometimes beyond) their growth limit (there are also some special cases where infinite limits are handled differently, such as a step when any tracks with finite growth limits are increased up to their limit). A negative infinity limit wouldn't make any sense (we'd be better off using 0), and as you note, the limit only becomes infinite by being explicitly set to f32::INFINITY.

The use of f32::INFINITY also directly corresponds to the use of "infinite growth limit" or "growth limit of infinity" in the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Make the code cleaner or prettier.
Projects
None yet
Development

No branches or pull requests

3 participants