ronantakizawa/codereview-bench huggingface dataset:
Repository: nodejs/node
File: src/node_process_events.cc (C++)

Code before:

#include "env-inl.h"
#include "node_process-inl.h"
#include "util.h"

namespace node {
using v8::Context;
using v8::Function;
using v8::HandleScope;
using v8::Isolate;
using v8::Just;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Nothing;
using v8::Object;
using v8::String;
using v8::Value;

Maybe<bool> ProcessEmitWarningSync(Environment* env, std::string_view message) {
  Isolate* isolate = env->isolate();
  Local<Context> context = env->context();
  Local<String> message_string;
  if (!String::NewFromUtf8(isolate,
                              message.data(),
                              NewStringType::kNormal,
                              static_cast<int>(message.size()))
          .ToLocal(&message_string)) {
    return Nothing<bool>();
  }

  Local<Value> argv[] = {message_string};
  Local<Function> emit_function = env->process_emit_warning_sync();
  // If this fails, this is called too early - before the bootstrap is even
  // finished.
  CHECK(!emit_function.IsEmpty());
  if (emit_function.As<Function>()
          ->Call(context, v8::Undefined(isolate), arraysize(argv), argv)
          .IsEmpty()) {
    return Nothing<bool>();
  }
  return Just(true);
}

MaybeLocal<Value> ProcessEmit(Environment* env,
                              std::string_view event,
                              Local<Value> message) {
  Isolate* isolate = env->isolate();

  Local<Value> event_string;
  if (!ToV8Value(env->context(), event).ToLocal(&event_string)) {

Review comment (suggestion):
You'll need to add the `using v8::NewStringType` to the list above for this to work :-) 

Code after:

#include "env-inl.h"
#include "node_process-inl.h"
#include "util.h"

namespace node {
using v8::Context;
using v8::Function;
using v8::HandleScope;
using v8::Isolate;
using v8::Just;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Nothing;
using v8::Object;
using v8::String;
using v8::Value;

Maybe<bool> ProcessEmitWarningSync(Environment* env, std::string_view message) {
  Isolate* isolate = env->isolate();
  Local<Context> context = env->context();
  Local<String> message_string;
  if (!String::NewFromUtf8(isolate,
                           message.data(),
                           NewStringType::kNormal,
                           static_cast<int>(message.size()))
           .ToLocal(&message_string)) {
    return Nothing<bool>();
  }

  Local<Value> argv[] = {message_string};
  Local<Function> emit_function = env->process_emit_warning_sync();
  // If this fails, this is called too early - before the bootstrap is even
  // finished.
  CHECK(!emit_function.IsEmpty());
  if (emit_function.As<Function>()
          ->Call(context, v8::Undefined(isolate), arraysize(argv), argv)
          .IsEmpty()) {
    return Nothing<bool>();
  }
  return Just(true);
}

MaybeLocal<Value> ProcessEmit(Environment* env,
                              std::string_view event,
                              Local<Value> message) {
  Isolate* isolate = env->isolate();

  Local<Value> event_string;
Repository: nodejs/node
File: test/parallel/test-fs-promises-watch-iterator.js (JavaScript)

Code before:
  async run() {
    await Promise.all([this.watchFiles(), this.writeFiles()]);
    assert(!this.files.length);
  }
  async watchFiles() {
    const watcher = watch(this.dirPath);
    for await (const evt of watcher) {
      const idx = this.files.indexOf(evt.filename);
      if (idx < 0) continue;
      this.files.splice(idx, 1);
      await setTimeout(common.platformTimeout(100));
      if (!this.files.length) break;
    }
  }
  async writeFiles() {
    for (const fileName of [...this.files]) {
      await writeFile(this.filePath(fileName), Date.now() + fileName.repeat(1e4));
    }
    await setTimeout(common.platformTimeout(100));
  }
}

const kCases = [
  // Watch on a directory should callback with a filename on supported systems
  new WatchTestCase(
    common.isLinux || common.isMacOS || common.isWindows || common.isAIX,
    'watch1',
    ['foo', 'bar', 'baz']
  ),
];

tmpdir.refresh();

for (const testCase of kCases) {
  if (testCase.shouldSkip) continue;
  fs.mkdirSync(testCase.dirPath);
  testCase.run().then(common.mustCall());
}


Review comment (suggestion):
Can you put the conditions in `test/parallel/parallel.status` instead? That way the file would be skipped completely on these platforms by the test runner (but could be run if necessary individually).

Code after:
    await Promise.all([this.watchFiles(), this.writeFiles()]);
    assert(!this.files.length);
  }
  async watchFiles() {
    const watcher = watch(this.dirPath);
    for await (const evt of watcher) {
      const idx = this.files.indexOf(evt.filename);
      if (idx < 0) continue;
      this.files.splice(idx, 1);
      await setTimeout(common.platformTimeout(100));
      if (!this.files.length) break;
    }
  }
  async writeFiles() {
    for (const fileName of [...this.files]) {
      await writeFile(this.filePath(fileName), Date.now() + fileName.repeat(1e4));
    }
    await setTimeout(common.platformTimeout(100));
  }
}

const kCases = [
  // Watch on a directory should callback with a filename on supported systems
  new WatchTestCase(
    'watch1',
    ['foo', 'bar', 'baz']
  ),
];

tmpdir.refresh();

for (const testCase of kCases) {
  fs.mkdirSync(testCase.dirPath);
  testCase.run().then(common.mustCall());
}
Repository: nodejs/node
File: test/parallel/test-fs-promises-watch-iterator.js (JavaScript)

Code before:
'use strict';
const common = require('../common');
const { watch, writeFile } = require('fs/promises');
const fs = require('fs');
const assert = require('assert');
const { join } = require('path');
const { setTimeout } = require('timers/promises');
const tmpdir = require('../common/tmpdir');

class WatchTestCase {
  constructor(dirName, files) {
    this.dirName = dirName;
    this.files = files;
  }
  get dirPath() { return tmpdir.resolve(this.dirName); }
  filePath(fileName) { return join(this.dirPath, fileName); }

  async run() {
    await Promise.all([this.watchFiles(), this.writeFiles()]);
    assert(!this.files.length);
  }
  async watchFiles() {
    const watcher = watch(this.dirPath);
    for await (const evt of watcher) {
      const idx = this.files.indexOf(evt.filename);
      if (idx < 0) continue;
      this.files.splice(idx, 1);
      await setTimeout(common.platformTimeout(100));
      if (!this.files.length) break;
    }
  }

Review comment (suggestion):
```suggestion
// This tests that when there is a burst of fs watch events, the events
// emitted after the consumer receives the initial event and before the
// control returns back to fs.watch() can be queued up and show up
// in the next iteration.
const common = require('../common');
```

Code after:
'use strict';
// This tests that when there is a burst of fs watch events, the events
// emitted after the consumer receives the initial event and before the
// control returns back to fs.watch() can be queued up and show up
// in the next iteration.
const common = require('../common');
const { watch, writeFile } = require('fs/promises');
const fs = require('fs');
const assert = require('assert');
const { join } = require('path');
const { setTimeout } = require('timers/promises');
const tmpdir = require('../common/tmpdir');

class WatchTestCase {
  constructor(dirName, files) {
    this.dirName = dirName;
    this.files = files;
  }
  get dirPath() { return tmpdir.resolve(this.dirName); }
  filePath(fileName) { return join(this.dirPath, fileName); }

  async run() {
    await Promise.all([this.watchFiles(), this.writeFiles()]);
    assert(!this.files.length);
  }
  async watchFiles() {
    const watcher = watch(this.dirPath);
    for await (const evt of watcher) {
      const idx = this.files.indexOf(evt.filename);
      if (idx < 0) continue;
      this.files.splice(idx, 1);
Repository: nodejs/node
File: lib/internal/fs/watchers.js (JavaScript)

Code before:
  const queue = [];
  const oncancel = () => {
    handle.close();
    resolve();
  };

  try {
    if (signal) {
      kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
      signal.addEventListener('abort', oncancel, { __proto__: null, once: true, [kResistStopPropagation]: true });
    }
    handle.onchange = (status, eventType, filename) => {
      if (status < 0) {
        const error = new UVException({
          errno: status,
          syscall: 'watch',
          path: filename,
        });
        error.filename = filename;
        handle.close();
        ArrayPrototypePush(queue, error);
        resolve();
        return;
      }
      if (queue.length < maxQueue) {
        ArrayPrototypePush(queue, { eventType, filename });
        resolve();
      } else if (overflow === 'throw') {
        const overflowError = genericNodeError('fs.watch maxQueue exceeded');
        queue.length = 0;
        ArrayPrototypePush(queue, overflowError);
        resolve();
      } else {
        process.emitWarning('fs.watch maxQueue exceeded');
      }
    };

    const err = handle.start(path, persistent, recursive, encoding);
    if (err) {
      const error = new UVException({
        errno: err,
        syscall: 'watch',
        path: filename,
        message: err === UV_ENOSPC ?
          'System limit for number of file watchers reached' : '',
      });
      error.filename = filename;
      handle.close();
      throw error;
    }


Review comment (suggestion):
```suggestion
        ArrayPrototypePush(queue, { __proto__: null, eventType, filename });
```

Code after:
  const queue = [];
  const oncancel = () => {
    handle.close();
    resolve();
  };

  try {
    if (signal) {
      kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
      signal.addEventListener('abort', oncancel, { __proto__: null, once: true, [kResistStopPropagation]: true });
    }
    handle.onchange = (status, eventType, filename) => {
      if (status < 0) {
        const error = new UVException({
          errno: status,
          syscall: 'watch',
          path: filename,
        });
        error.filename = filename;
        handle.close();
        ArrayPrototypePush(queue, error);
        resolve();
        return;
      }
      if (queue.length < maxQueue) {
        ArrayPrototypePush(queue, { __proto__: null, eventType, filename });
        resolve();
      } else if (overflow === 'error') {
        queue.length = 0;
        ArrayPrototypePush(queue, new ERR_FS_WATCH_QUEUE_OVERFLOW(maxQueue));
        resolve();
      } else {
        process.emitWarning('fs.watch maxQueue exceeded');
      }
    };

    const err = handle.start(path, persistent, recursive, encoding);
    if (err) {
      const error = new UVException({
        errno: err,
        syscall: 'watch',
        path: filename,
        message: err === UV_ENOSPC ?
          'System limit for number of file watchers reached' : '',
      });
      error.filename = filename;
      handle.close();
      throw error;
    }

    while (!signal?.aborted) {
Repository: nodejs/node
File: lib/internal/fs/watchers.js (JavaScript)

Code before:
        ArrayPrototypePush(queue, overflowError);
        resolve();
      } else {
        process.emitWarning('fs.watch maxQueue exceeded');
      }
    };

    const err = handle.start(path, persistent, recursive, encoding);
    if (err) {
      const error = new UVException({
        errno: err,
        syscall: 'watch',
        path: filename,
        message: err === UV_ENOSPC ?
          'System limit for number of file watchers reached' : '',
      });
      error.filename = filename;
      handle.close();
      throw error;
    }

    while (!signal?.aborted) {
      await promise;
      while (queue.length) {
        const item = ArrayPrototypeShift(queue);
        if (
          ObjectPrototypeHasOwnProperty(item, 'eventType') &&
          ObjectPrototypeHasOwnProperty(item, 'filename')
        ) {
          yield item;
        } else {
          throw item;
        }
      }
      ({ promise, resolve } = PromiseWithResolvers());
    }
    if (signal?.aborted) {
      throw new AbortError(undefined, { cause: signal?.reason });
    }
  } finally {
    handle.close();
    signal?.removeEventListener('abort', oncancel);
  }
}

module.exports = {
  FSWatcher,
  StatWatcher,
  kFSWatchStart,
  kFSStatWatcherStart,
  kFSStatWatcherAddOrCleanRef,

Review comment (suggestion):
```suggestion
        if (item instanceof Error) {
          throw item;
        } else {
          yield item;
```

I think this should be enough - would've been better if we have `Error.isError` but then I don't think it's possible for things from another realm to end up here.

Code after:
        resolve();
      } else {
        process.emitWarning('fs.watch maxQueue exceeded');
      }
    };

    const err = handle.start(path, persistent, recursive, encoding);
    if (err) {
      const error = new UVException({
        errno: err,
        syscall: 'watch',
        path: filename,
        message: err === UV_ENOSPC ?
          'System limit for number of file watchers reached' : '',
      });
      error.filename = filename;
      handle.close();
      throw error;
    }

    while (!signal?.aborted) {
      await promise;
      while (queue.length) {
        const item = ArrayPrototypeShift(queue);
        if (item instanceof Error) {
          throw item;
        } else {
          yield item;
        }
      }
      ({ promise, resolve } = PromiseWithResolvers());
    }
    if (signal?.aborted) {
      throw new AbortError(undefined, { cause: signal?.reason });
    }
  } finally {
    handle.close();
    signal?.removeEventListener('abort', oncancel);
  }
}

module.exports = {
  FSWatcher,
  StatWatcher,
  kFSWatchStart,
  kFSStatWatcherStart,
  kFSStatWatcherAddOrCleanRef,
  watch,
};
Repository: nodejs/node
File: test/parallel/test-fs-promises-watch.js (JavaScript)

Code before:
assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { signal: 1 })) { }
  },
  { code: 'ERR_INVALID_ARG_TYPE' }).then(common.mustCall());

assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { maxQueue: 'silly' })) { }
  },
  { code: 'ERR_INVALID_ARG_TYPE' }).then(common.mustCall());

assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { overflow: 1 })) { }
  },
  { code: 'ERR_INVALID_ARG_VALUE' }).then(common.mustCall());

assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { overflow: 'barf' })) { }
  },
  { code: 'ERR_INVALID_ARG_VALUE' }).then(common.mustCall());

(async () => {
  const ac = new AbortController();
  const { signal } = ac;
  setImmediate(() => ac.abort());
  try {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch(__filename, { signal })) { }
  } catch (err) {
    assert.strictEqual(err.name, 'AbortError');
  }
})().then(common.mustCall());


Review comment (bug):
The previous tests are actually not ideal to follow because like all the other async fs APIs, `fs.watch()` throws validation errors synchounously so there's no need to use `assert.rejects`.

```suggestion
assert.throws(() => watch('', { maxQueue: 'silly' }), { code: 'ERR_INVALID_ARG_TYPE' });
assert.throws(() => watch('', { overflow: 1 }), { code: 'ERR_INVALID_ARG_TYPE' });
assert.throws(() => watch('', { overflow: 'barf' }), { code: 'ERR_INVALID_ARG_TYPE' });
```

Code after:
assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { signal: 1 })) { }
  },
  { code: 'ERR_INVALID_ARG_TYPE' }).then(common.mustCall());

assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { maxQueue: 'silly' })) { }
  },
  { code: 'ERR_INVALID_ARG_TYPE' }).then(common.mustCall());
assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { overflow: 1 })) { }
  },
  { code: 'ERR_INVALID_ARG_VALUE' }).then(common.mustCall());
assert.rejects(
  async () => {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch('', { overflow: 'barf' })) { }
  },
  { code: 'ERR_INVALID_ARG_VALUE' }).then(common.mustCall());

(async () => {
  const ac = new AbortController();
  const { signal } = ac;
  setImmediate(() => ac.abort());
  try {
    // eslint-disable-next-line no-unused-vars, no-empty
    for await (const _ of watch(__filename, { signal })) { }
  } catch (err) {
    assert.strictEqual(err.name, 'AbortError');
  }
})().then(common.mustCall());
Repository: nodejs/node
File: src/js_native_api_types.h (C/C++)

Code before:
#endif  // NAPI_VERSION >= 8
} napi_property_attributes;

typedef enum {
  // ES6 types (corresponds to typeof)
  napi_undefined,
  napi_null,
  napi_boolean,
  napi_number,
  napi_string,
  napi_symbol,
  napi_object,
  napi_function,
  napi_external,
  napi_bigint,
} napi_valuetype;

typedef enum {
  napi_int8_array,
  napi_uint8_array,
  napi_uint8_clamped_array,
  napi_int16_array,
  napi_uint16_array,
  napi_int32_array,
  napi_uint32_array,
  napi_float16_array,
  napi_float32_array,
  napi_float64_array,
  napi_bigint64_array,
  napi_biguint64_array,
} napi_typedarray_type;

typedef enum {
  napi_ok,
  napi_invalid_arg,
  napi_object_expected,
  napi_string_expected,
  napi_name_expected,
  napi_function_expected,
  napi_number_expected,
  napi_boolean_expected,
  napi_array_expected,
  napi_generic_failure,
  napi_pending_exception,
  napi_cancelled,
  napi_escape_called_twice,
  napi_handle_scope_mismatch,
  napi_callback_scope_mismatch,
  napi_queue_full,
  napi_closing,
  napi_bigint_expected,

Review comment (suggestion):
It is important to note that Node-API is an ABI stable API. It means that any code that was compiled before with the previous version of Node-API must work in the future version of Node-API. See more details here: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md

By inserting a new enum value in the middle of the existing enum values we effectively change integer values for all enum values that follow the inserted entry. Any previously compiled code that used to create float32 array will suddenly create the float16 array, float64 became float32, etc.

The only valid place to insert new enum values in Node-API is at the end of an enum.

Also, any Node-API changes must be versioned and initially be part of the experimental version. 
I.e. the new enum entry must be under `#ifdef NAPI_EXPERIMENTAL`. E.g. see [this commit](https://github.com/nodejs/node/commit/c9506a8f3e9bc5c679151feb39198023154464ab) where the new Node-API property attributes were added.

Code after:
#endif  // NAPI_VERSION >= 8
} napi_property_attributes;

typedef enum {
  // ES6 types (corresponds to typeof)
  napi_undefined,
  napi_null,
  napi_boolean,
  napi_number,
  napi_string,
  napi_symbol,
  napi_object,
  napi_function,
  napi_external,
  napi_bigint,
} napi_valuetype;

typedef enum {
  napi_int8_array,
  napi_uint8_array,
  napi_uint8_clamped_array,
  napi_int16_array,
  napi_uint16_array,
  napi_int32_array,
  napi_uint32_array,
  napi_float32_array,
  napi_float64_array,
  napi_bigint64_array,
  napi_biguint64_array,
#define NODE_API_HAS_FLOAT16_ARRAY
  napi_float16_array,
} napi_typedarray_type;

typedef enum {
  napi_ok,
  napi_invalid_arg,
  napi_object_expected,
  napi_string_expected,
  napi_name_expected,
  napi_function_expected,
  napi_number_expected,
  napi_boolean_expected,
  napi_array_expected,
  napi_generic_failure,
  napi_pending_exception,
  napi_cancelled,
  napi_escape_called_twice,
  napi_handle_scope_mismatch,
  napi_callback_scope_mismatch,
  napi_queue_full,
  napi_closing,
Repository: nodejs/node
File: lib/internal/crypto/hash.js (JavaScript)

Code before:
    ERR_CRYPTO_HASH_UPDATE_FAILED,
    ERR_INVALID_ARG_TYPE,
    ERR_INVALID_ARG_VALUE,
  },
} = require('internal/errors');

const {
  validateEncoding,
  validateString,
  validateUint32,
} = require('internal/validators');

const {
  isArrayBufferView,
} = require('internal/util/types');

const LazyTransform = require('internal/streams/lazy_transform');

const kState = Symbol('kState');
const kFinalized = Symbol('kFinalized');

/**
 * @param {string} name
 */
function normalizeAlgorithm(name) {
  return name.toLowerCase().replace('-', '');
}

let warned = false;
const emitDeprecationWarning = getDeprecationWarningEmitter(
  'DEP0198',
  'Creating SHAKE128/256 digests without an explicit options.outputLength is deprecated.',
  undefined,
  false,
  isPendingDeprecation,
);

function Hash(algorithm, options) {
  if (!new.target)
    return new Hash(algorithm, options);
  const isCopy = algorithm instanceof _Hash;
  if (!isCopy)
    validateString(algorithm, 'algorithm');
  const xofLen = typeof options === 'object' && options !== null ?
    options.outputLength : undefined;
  if (xofLen !== undefined)
    validateUint32(xofLen, 'options.outputLength');
  // Lookup the cached ID from JS land because it's faster than decoding
  // the string in C++ land.
  const algorithmId = isCopy ? -1 : getCachedHashId(algorithm);
  this[kHandle] = new _Hash(algorithm, xofLen, algorithmId, getHashCache());

Review comment (suggestion):
```suggestion
  return StringPrototypeReplace(StringPrototypeToLowerCase(name), '-', '');
```

Code after:
    ERR_CRYPTO_HASH_FINALIZED,
    ERR_CRYPTO_HASH_UPDATE_FAILED,
    ERR_INVALID_ARG_TYPE,
    ERR_INVALID_ARG_VALUE,
  },
} = require('internal/errors');

const {
  validateEncoding,
  validateString,
  validateUint32,
} = require('internal/validators');

const {
  isArrayBufferView,
} = require('internal/util/types');

const LazyTransform = require('internal/streams/lazy_transform');

const kState = Symbol('kState');
const kFinalized = Symbol('kFinalized');

/**
 * @param {string} name
 */
function normalizeAlgorithm(name) {
  return StringPrototypeReplace(StringPrototypeToLowerCase(name), '-', '');
}

const maybeEmitDeprecationWarning = getDeprecationWarningEmitter(
  'DEP0198',
  'Creating SHAKE128/256 digests without an explicit options.outputLength is deprecated.',
  undefined,
  false,
  (algorithm) => {
    if (!isPendingDeprecation()) return false;
    const normalized = normalizeAlgorithm(algorithm);
    return normalized === 'shake128' || normalized === 'shake256';
  },
);

function Hash(algorithm, options) {
  if (!new.target)
    return new Hash(algorithm, options);
  const isCopy = algorithm instanceof _Hash;
  if (!isCopy)
    validateString(algorithm, 'algorithm');
  const xofLen = typeof options === 'object' && options !== null ?
    options.outputLength : undefined;
  if (xofLen !== undefined)
    validateUint32(xofLen, 'options.outputLength');