I wonder - is this a rare case where having an extension on double would actually be better than a standalone function?

I wonder - is this a rare case where having an extension on double would actually be better than a standalone function?

It definitely would have made my life easier but it's my understanding it's against the style guide.

It would be nice to have a test that ensures anyone that commits new code uses clampDouble instead of num.clamp.

See https://github.com/flutter/flutter/blob/master/dev/bots/analyze.dart for some examples of flagging code or comment patterns to fail analysis

Yea, I thought the same thing. I read through that script and the only step that is actually parsing any code is verifySkipTestComments which is parsing files based on the name ending with _test.dart. We need to fully parse the files in order to distinguish between clamp called on ints and clamp called on doubles. I would need to do a bit of trailblazing to make that work. It's worth probably time boxing some time to at least take a stab at it. I'll probably have to juggle things around so different steps don't parse the same files twice. This thing is just used to run a CI step once per PR?

@jonahwilliams does that analyze bot get run anywhere? It fails for me locally:

aaclarke-macbookpro2:flutter aaclarke$ git checkout master
Switched to branch 'master'
Your branch is behind 'origin/master' by 323 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)
aaclarke-macbookpro2:flutter aaclarke$ dart --enable-asserts run dev/bots/analyze.dart 
▌16:33:34▐ STARTING ANALYSIS
▌16:33:34▐ All tool test files end in _test.dart...
▌16:33:34▐ No sync*/async*
▌16:33:35▐ No runtimeType in toString...
▌16:33:35▐ Debug mode instead of checked mode...
▌16:33:36▐ Links for creating GitHub issues
▌16:33:37▐ Unexpected binaries...
▌16:33:38▐ Trailing spaces...
▌16:33:39▐ Deprecations...
▌16:33:40▐ Goldens...
▌16:33:41▐ Skip test comments...
Unhandled exception:
Invalid argument(s): Content produced diagnostics when parsed:
  EXPECTED_TOKEN: Expected to find '}'. - 54:18

#0      parseString (package:analyzer/dart/analysis/utilities.dart:102:5)
#1      parseFile (package:analyzer/dart/analysis/utilities.dart:46:10)
#2      _getTestSkips (file:///Users/aaclarke/dev/flutter/dev/bots/analyze.dart:528:41)
#3      verifySkipTestComments (file:///Users/aaclarke/dev/flutter/dev/bots/analyze.dart:573:34)
<asynchronous suspension>
#4      run (file:///Users/aaclarke/dev/flutter/dev/bots/analyze.dart:120:3)
<asynchronous suspension>
#5      main (file:///Users/aaclarke/dev/flutter/dev/bots/analyze.dart:52:5)
<asynchronous suspension>

It runs on every CI. Try dart --enable-asserts dev/bots/analyze.dart

It would be nice to have a lint for this somehow. I don't think we can properly lint this without having an analyzer check for it.

It would also be nice if somehow the compiler can just fix this for us via TFA. @alexmarkov @a-siva

Please add a microbenchmark for this.

Microbenchmark added, in microbenchmarks it's actually 80x faster. (I was guessing 4x based off of what I was seeing in profiling).

(rebasing to hopefully flush out outstanding ci issues)

I'm landing this despite google testing showing up yellow. The google test was run successfully in the above CL, the rerun that just happened finished but didn't update its status yet. There is one failure there out of 2190 runs. There is one failure that isn't related to this one, but has to do with apple signing certificates.

I guess every single one of you has thought about this possibility, but since no one said anything, isn't it possible to fix Dart? Or at least do something there like clamp2/dangerousClamp/fastClamp, who knows? lol This is super useful. If it is too big, maybe schedule for Dart 3? I would love to see this on Dart side too.

I guess every single one of you has thought about this possibility, but since no one said anything, isn't it possible to fix Dart? Or at least do something there like clamp2/dangerousClamp/fastClamp, who knows? lol This is super useful. If it is too big, maybe schedule for Dart 3? I would love to see this on Dart side too.

I'm going to file a bug with them but if you look at the the reason that this is faster, not all of them could be brought into Dart. For example we ignore nan for the limits. We can do that because we know we don't care about it. Dart can't do that. That said, my understanding is that the large part of this optimization comes from avoiding num which requires polymorphism to evaluate. A solution probably requires a special case for compile-time function resolution when dealing with num types.

FYI @mraleph

@bernaferrari we are going to fix Dart (see dart-lang/sdk#40852 and dart-lang/sdk#46879), it's not going to become as fast as custom clamp implementation (because it does more checking), but we should get pretty close.

Note that while improvement looks great/important in isolation, it hardly matters unless all you do is clamping.

This PR resulted in a 4.2% increase in performance for stock_layout_iteration: https://flutter-flutter-perf.skia.org/e/?begin=1652777644&end=1652981286&queries=sub_result%3Dstock_layout_iteration&requestType=0