ronantakizawa/codereview-bench huggingface dataset:
Repository: flutter/flutter
File: packages/flutter/lib/src/painting/text_scaler.dart (Dart)

Code before:
  final double minScale;
  final double maxScale;

  @override
  double get textScaleFactor => clampDouble(scaler.textScaleFactor, minScale, maxScale);

  @override
  double scale(double fontSize) {
    assert(fontSize >= 0);
    assert(fontSize.isFinite);
    return clampDouble(scaler.scale(fontSize), minScale * fontSize, maxScale * fontSize);
  }

  @override
  TextScaler clamp({double minScaleFactor = 0, double maxScaleFactor = double.infinity}) {
    final double newMinScale = max(minScale, minScaleFactor);
    final double newMaxScale = min(maxScale, maxScaleFactor);

    if (newMaxScale <= newMinScale) {
      // Ranges don't overlap or collapse to a single point.
      return TextScaler.linear(clampDouble(minScale, minScaleFactor, maxScaleFactor));
    }
    return _ClampedTextScaler(scaler, newMinScale, newMaxScale);
  }

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) {
      return true;
    }
    return other is _ClampedTextScaler &&
        minScale == other.minScale &&
        maxScale == other.maxScale &&
        scaler == other.scaler;
  }

  @override
  int get hashCode => Object.hash(scaler, minScale, maxScale);

  @override
  String toString() => '$scaler clamped [$minScale, $maxScale]';
}


Review comment (suggestion):
When `newMaxScale < newMinScale` I don't think this matches the API docs: https://main-api.flutter.dev/flutter/painting/TextScaler/clamp.html

Code after:
  final double minScale;
  final double maxScale;

  @override
  double get textScaleFactor => clampDouble(scaler.textScaleFactor, minScale, maxScale);

  @override
  double scale(double fontSize) {
    assert(fontSize >= 0);
    assert(fontSize.isFinite);
    return clampDouble(scaler.scale(fontSize), minScale * fontSize, maxScale * fontSize);
  }

  @override
  TextScaler clamp({double minScaleFactor = 0, double maxScaleFactor = double.infinity}) {
    assert(maxScaleFactor >= minScaleFactor);
    assert(!maxScaleFactor.isNaN);
    assert(minScaleFactor.isFinite);
    assert(minScaleFactor >= 0);

    final double newMinScale = max(minScale, minScaleFactor);
    final double newMaxScale = min(maxScale, maxScaleFactor);

    if (newMaxScale <= newMinScale) {
      // Ranges don't overlap or collapse to a single point.
      return TextScaler.linear(clampDouble(minScale, minScaleFactor, maxScaleFactor));
    }
    return _ClampedTextScaler(scaler, newMinScale, newMaxScale);
  }

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) {
      return true;
    }
    return other is _ClampedTextScaler &&
        minScale == other.minScale &&
        maxScale == other.maxScale &&
        scaler == other.scaler;
  }

  @override
  int get hashCode => Object.hash(scaler, minScale, maxScale);

  @override
  String toString() => '$scaler clamped [$minScale, $maxScale]';
}
Repository: flutter/flutter
File: packages/flutter/lib/src/widgets/icon_data.dart (Dart)

Code before:
/// Annotation for classes that only provide static const [IconData] instances.
///
/// This is a hint to the font tree shaker to ignore the constant instances
/// of [IconData] appearing in the declaration of this class when tree-shaking
/// unused code points from the bundled font.
///
/// Classes with this annotation must have only "static const" members. The
/// presence of any non-const [IconData] instances will preclude apps
/// importing the declaration into their application from being able to use
/// icon tree-shaking during release builds, resulting in larger font assets.
///
/// ```dart
/// @staticIconProvider
/// abstract final class MyCustomIcons {
///   static const String fontFamily = 'MyCustomIcons';
///   static const IconData happyFace = IconData(1, fontFamily: fontFamily);
///   static const IconData sadFace = IconData(2, fontFamily: fontFamily);
/// }
/// ```
const Object staticIconProvider = _StaticIconProvider();

/// Retain the fields of [IconData] to ensure the icon tree shaker can access
/// those fields after tree shaking the application.
///
/// NOTE: We may replace this with a `@RecordUse()` annotation once icon tree
/// shaking uses the new link hooks.
const _retainForIconTreeShaker = pragma('vm:entry-point');


Review comment (suggestion):
Nit:

"
This may be replaced with a `@RecordUse()` annotation once icon tree shaking uses the new line hooks.
"

https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-empty-prose

https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#use-the-passive-voice-recommend-do-not-require-never-say-things-are-simple

Code after:
/// Annotation for classes that only provide static const [IconData] instances.
///
/// This is a hint to the font tree shaker to ignore the constant instances
/// of [IconData] appearing in the declaration of this class when tree-shaking
/// unused code points from the bundled font.
///
/// Classes with this annotation must have only "static const" members. The
/// presence of any non-const [IconData] instances will preclude apps
/// importing the declaration into their application from being able to use
/// icon tree-shaking during release builds, resulting in larger font assets.
///
/// ```dart
/// @staticIconProvider
/// abstract final class MyCustomIcons {
///   static const String fontFamily = 'MyCustomIcons';
///   static const IconData happyFace = IconData(1, fontFamily: fontFamily);
///   static const IconData sadFace = IconData(2, fontFamily: fontFamily);
/// }
/// ```
const Object staticIconProvider = _StaticIconProvider();

/// Retain the fields of [IconData] to ensure the icon tree shaker can access
/// those fields after tree shaking the application.
///
/// This may be replaced with a `@RecordUse()` annotation once icon tree
/// shaking uses the new link hooks.
const _retainForIconTreeShaker = pragma('vm:entry-point');
Repository: flutter/flutter
File: packages/flutter/test/widgets/transformed_scrollable_test.dart (Dart)

Code before:
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:math' as math;

import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';

import 'widgets_app_tester.dart';

/// The blue color used in this test, equivalent to `Colors.blue`.
const Color _kBlue = Color(0xFF2196F3);
/// The red color used in this test, equivalent to `Colors.red`.
const Color _kRed = Color(0xFFF44336);

void main() {
  testWidgets('Scrollable scaled up', (WidgetTester tester) async {
    final controller = ScrollController();
    addTearDown(controller.dispose);

    await tester.pumpWidget(
      TestWidgetsApp(
        home: Transform.scale(
          scale: 2.0,
          child: Center(
            child: SizedBox(
              width: 200,
              child: ListView.builder(
                controller: controller,
                cacheExtent: 0.0,
                itemBuilder: (BuildContext context, int index) {
                  return Container(
                    height: 100.0,
                    color: index.isEven ? _kBlue : _kRed,
                    child: Text('Tile $index'),
                  );
                },

Review comment (suggestion):
The tests don't actually require a specific color it seems? Can you use single channel colors, to make the test easier to read? Then the comments can also be omitted, since these no longer apply.

```
const Color _kBlue = Color(0xFF0000FF);
const Color _kRed = Color(0xFFFFF0000);
```

Code after:
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:math' as math;

import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';

import 'widgets_app_tester.dart';

const Color _kBlue = Color(0xFF0000FF);
const Color _kRed = Color(0xFFFF0000);

void main() {
  testWidgets('Scrollable scaled up', (WidgetTester tester) async {
    final controller = ScrollController();
    addTearDown(controller.dispose);

    await tester.pumpWidget(
      TestWidgetsApp(
        home: Transform.scale(
          scale: 2.0,
          child: Center(
            child: SizedBox(
              width: 200,
              child: ListView.builder(
                controller: controller,
                cacheExtent: 0.0,
                itemBuilder: (BuildContext context, int index) {
                  return Container(
                    height: 100.0,
                    color: index.isEven ? _kBlue : _kRed,
                    child: Text('Tile $index'),
                  );
                },
              ),
            ),
Repository: flutter/flutter
File: packages/flutter_tools/lib/src/base/utils.dart (Dart)

Code before:
);

/// Formats a list of rows into a table with aligned columns.
///
/// [table] is a list of rows, where each row is a list of strings.
/// [separator] is the string used to separate columns (default is ' • ').
///
/// Returns a list of strings, where each string is a formatted row.
List<String> formatTable(List<List<String>> table, {String separator = ' • ', int indent = 0}) {
  if (table.isEmpty) {
    return <String>[];
  }

  // Calculate column widths
  final indices = List<int>.generate(table[0].length - 1, (int i) => i);
  List<int> widths = indices.map<int>((int i) => 0).toList();
  for (final row in table) {
    widths = indices.map<int>((int i) => math.max(widths[i], row[i].length)).toList();
  }

  final String indentString = ' ' * indent;

  // Join columns into lines of text
  return <String>[
    for (final List<String> row in table)
      '$indentString${indices.map<String>((int i) => row[i].padRight(widths[i])).followedBy(<String>[row.last]).join(separator)}',
  ];
}


Review comment (suggestion):
This is a bit hard to read. How about this?

```dart
  return table.map<String>((row) {
    final formatted = indices
        .map<String>((int i) => row[i].padRight(widths[i]))
        .followedBy(<String>[row.last])
        .join();
    return '$indentString$formatted';
  }).toList();
```

Code after:
);

/// Formats a list of rows into a table with aligned columns.
///
/// [table] is a list of rows, where each row is a list of strings.
/// [separator] is the string used to separate columns (default is ' • ').
///
/// Returns a list of strings, where each string is a formatted row.
List<String> formatTable(List<List<String>> table, {String separator = ' • ', int indent = 0}) {
  if (table.isEmpty) {
    return <String>[];
  }

  // Calculate column widths
  if (table.first.isEmpty) {
    throw Exception('Table header cannot be empty');
  }
  final indices = List<int>.generate(table.first.length - 1, (int i) => i);
  final widths = List<int>.filled(indices.length, 0);
  for (final row in table) {
    for (final i in indices) {
      widths[i] = math.max(widths[i], row[i].length);
    }
  }

  final String indentString = ' ' * indent;

  // Join columns into lines of text
  return table.map<String>((List<String> row) {
    final String formatted = indices
        .map<String>((int i) => row[i].padRight(widths[i]))
        .followedBy(<String>[row.last])
        .join(separator);
    return '$indentString$formatted';
  }).toList();
}
Repository: flutter/flutter
File: packages/flutter_tools/test/general.shard/web_template_test.dart (Dart)

Code before:
      environment: '{{ENV}}',
      debugMode: {{DEBUG_MODE}}
    };
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithWebDefines);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{
        'API_URL': 'https://api.example.com',
        'ENV': 'production',
        'DEBUG_MODE': 'false',
      },
      logger: logger,
    );

    expect(result, contains("apiUrl: 'https://api.example.com'"));
    expect(result, contains("environment: 'production'"));
    expect(result, contains('debugMode: false'));
  });

  testUsingContext('logs warning when web-define variable is missing', () {
    const htmlWithMissingVar = '''
<!DOCTYPE html>
<html>
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    const apiUrl = '{{API_URL}}';
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMissingVar);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{}, // Missing API_URL
      logger: testLogger,
    );

    expect(testLogger.warningText, contains('Missing web-define variable: API_URL'));
    // Verify the placeholder is preserved

Review comment (suggestion):
```suggestion
  testUsingContext('logs warning when user defined web-define variable is missing', () {
```

Code after:
      environment: '{{ENV}}',
      debugMode: {{DEBUG_MODE}}
    };
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithWebDefines);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{
        'API_URL': 'https://api.example.com',
        'ENV': 'production',
        'DEBUG_MODE': 'false',
      },
      logger: logger,
    );

    expect(result, contains("apiUrl: 'https://api.example.com'"));
    expect(result, contains("environment: 'production'"));
    expect(result, contains('debugMode: false'));
  });

  testUsingContext('logs warning when user defined web-define variable is missing', () {
    const htmlWithMissingVar = '''
<!DOCTYPE html>
<html>
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    const apiUrl = '{{API_URL}}';
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMissingVar);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{}, // Missing API_URL
      logger: testLogger,
    );

    expect(testLogger.warningText, contains('Missing web-define variable: API_URL'));
    // Verify the placeholder is preserved
Repository: flutter/flutter
File: packages/flutter_tools/test/general.shard/web_template_test.dart (Dart)

Code before:
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    const apiUrl = '{{API_URL}}';
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMissingVar);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{}, // Missing API_URL
      logger: testLogger,
    );

    expect(testLogger.warningText, contains('Missing web-define variable: API_URL'));
    // Verify the placeholder is preserved
    expect(result, contains("const apiUrl = '{{API_URL}}';"));
  });

  testUsingContext('logs warning with multiple missing variables', () {
    const htmlWithMultipleMissingVars = '''
<!DOCTYPE html>
<html>
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    window.config = {
      api: '{{API_URL}}',
      env: '{{ENV}}',
      version: '{{VERSION}}'
    };
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMultipleMissingVars);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{'API_URL': 'test'}, // Missing ENV, VERSION
      logger: testLogger,

Review comment (suggestion):
```suggestion
  testUsingContext('logs warning with multiple missing user defined variables', () {
```

Code after:
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    const apiUrl = '{{API_URL}}';
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMissingVar);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{}, // Missing API_URL
      logger: testLogger,
    );

    expect(testLogger.warningText, contains('Missing web-define variable: API_URL'));
    // Verify the placeholder is preserved
    expect(result, contains("const apiUrl = '{{API_URL}}';"));
  });

  testUsingContext('logs warning with multiple missing user defined variables', () {
    const htmlWithMultipleMissingVars = '''
<!DOCTYPE html>
<html>
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    window.config = {
      api: '{{API_URL}}',
      env: '{{ENV}}',
      version: '{{VERSION}}'
    };
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMultipleMissingVars);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{'API_URL': 'test'}, // Missing ENV, VERSION
      logger: testLogger,
Repository: flutter/flutter
File: packages/flutter_tools/test/general.shard/web_template_test.dart (Dart)

Code before:
  <script>
    window.config = {
      api: '{{API_URL}}',
      env: '{{ENV}}',
      version: '{{VERSION}}'
    };
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMultipleMissingVars);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{'API_URL': 'test'}, // Missing ENV, VERSION
      logger: testLogger,
    );

    expect(testLogger.warningText, contains('Missing web-define variables: ENV, VERSION'));
    expect(result, contains("env: '{{ENV}}'"));
    expect(result, contains("version: '{{VERSION}}'"));
  });

  testUsingContext(
    'ignores Flutter built-in variables and logs logs warning for missing user variables',
    () {
      const htmlWithBuiltInVars = '''
<!DOCTYPE html>
<html>
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    {{flutter_js}}
    {{flutter_build_config}}
    const customVar = '{{CUSTOM_VAR}}';
  </script>
</body>
</html>''';

      const indexHtml = WebTemplate(htmlWithBuiltInVars);
      final String result = indexHtml.withSubstitutions(
        baseHref: '/',
        serviceWorkerVersion: null,
        flutterJsFile: flutterJs,
        buildConfig: 'test config',
        webDefines: <String, String>{}, // Missing CUSTOM_VAR but built-in vars should be ignored
        logger: testLogger,

Review comment (suggestion):
Typo :)

```suggestion
    'ignores Flutter built-in variables and logs warning for missing user variables',
```

Code after:
  <script>
    window.config = {
      api: '{{API_URL}}',
      env: '{{ENV}}',
      version: '{{VERSION}}'
    };
  </script>
</body>
</html>''';

    const indexHtml = WebTemplate(htmlWithMultipleMissingVars);
    final String result = indexHtml.withSubstitutions(
      baseHref: '/',
      serviceWorkerVersion: null,
      flutterJsFile: flutterJs,
      webDefines: <String, String>{'API_URL': 'test'}, // Missing ENV, VERSION
      logger: testLogger,
    );

    expect(testLogger.warningText, contains('Missing web-define variables: ENV, VERSION'));
    expect(result, contains("env: '{{ENV}}'"));
    expect(result, contains("version: '{{VERSION}}'"));
  });

  testUsingContext(
    'ignores Flutter built-in variables and logs warning for missing user variables',
    () {
      const htmlWithBuiltInVars = '''
<!DOCTYPE html>
<html>
<head>
  <title>Test</title>
  <base href="/">
</head>
<body>
  <script>
    {{flutter_js}}
    {{flutter_build_config}}
    const customVar = '{{CUSTOM_VAR}}';
  </script>
</body>
</html>''';

      const indexHtml = WebTemplate(htmlWithBuiltInVars);
      final String result = indexHtml.withSubstitutions(
        baseHref: '/',
        serviceWorkerVersion: null,
        flutterJsFile: flutterJs,
        buildConfig: 'test config',
        webDefines: <String, String>{}, // Missing CUSTOM_VAR but built-in vars should be ignored
        logger: testLogger,
Repository: fmtlib/fmt
File: include/fmt/printf.h (C/C++)

Code before:
      if (c == '0')
        spec.fill_ = '0';
      if (value != 0) {
        // Nonzero value means that we parsed width and don't need to
        // parse it or flags again, so return now.
        spec.width_ = value;
        return arg_index;
      }
    }
  }
  parse_flags(spec, it, end);
  // Parse width.
  if (it != end) {
    if (*it >= '0' && *it <= '9') {
      internal::error_handler eh;
      spec.width_ = parse_nonnegative_int(it, end, eh);
    } else if (*it == '*') {
      ++it;
      spec.width_ = visit_format_arg(
            internal::printf_width_handler<char_type>(spec), get_arg());
    }
  }
  return arg_index;
}

template <typename OutputIt, typename Char, typename AF>
OutputIt basic_printf_context<OutputIt, Char, AF>::format() {
  auto out = this->out();
  const auto range = this->parse_context();
  const Char * const end = range.end();
  for (const Char *it = range.begin(); it != end; ) {
    char_type c = *it++;
    if (c != '%') {
      *(out++) = c;
      continue;
    }
    if (it != end && *it == c) {
      ++it;
      *(out++) = c;
      continue;
    }

    format_specs spec;
    spec.align_ = ALIGN_RIGHT;

    // Parse argument index, flags and width.
    unsigned arg_index = parse_header(it, end, spec);

    // Parse precision.
    if (it != end && *it == '.') {
      ++it;

Review comment (performance):
I think it's better to keep the old logic of writing ranges of characters rather than character by character. Just replace `buffer.append` with `std::copy`. This will be easier to optimize so that reallocations are minimized.

Code after:
  parse_flags(spec, it, end);
  // Parse width.
  if (it != end) {
    if (*it >= '0' && *it <= '9') {
      internal::error_handler eh;
      spec.width_ = parse_nonnegative_int(it, end, eh);
    } else if (*it == '*') {
      ++it;
      spec.width_ = visit_format_arg(
          internal::printf_width_handler<char_type>(spec), get_arg());
    }
  }
  return arg_index;
}

template <typename OutputIt, typename Char, typename AF>
OutputIt basic_printf_context<OutputIt, Char, AF>::format() {
  auto out = this->out();
  const auto range = this->parse_context();
  const Char* const end = range.end();
  const Char* start = range.begin();
  auto it = start;
  while (it != end) {
    char_type c = *it++;
    if (c != '%') continue;
    if (it != end && *it == c) {
      out = std::copy(start, it, out);
      start = ++it;
      continue;
    }
    out = std::copy(start, it - 1, out);

    format_specs spec;
    spec.align_ = ALIGN_RIGHT;

    // Parse argument index, flags and width.
    unsigned arg_index = parse_header(it, end, spec);

    // Parse precision.
    if (it != end && *it == '.') {
      ++it;
      c = it != end ? *it : 0;
      if ('0' <= c && c <= '9') {
        internal::error_handler eh;
        spec.precision = static_cast<int>(parse_nonnegative_int(it, end, eh));
      } else if (c == '*') {
        ++it;
        spec.precision =
            visit_format_arg(internal::printf_precision_handler(), get_arg());
      } else {
        spec.precision = 0;