ronantakizawa/codereview-bench huggingface dataset:
Repository: mandiant/capa
File: capa/features/extractors/binja/helpers.py (Python)

Code before:
    # https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170#FormatC
    # Possible variations for BaseThreadInitThunk:
    # @BaseThreadInitThunk@12
    # _BaseThreadInitThunk
    # _BaseThreadInitThunk@12
    # It is also possible for a function to have a `Stub` appended to its name:
    # _lstrlenWStub@4

    # A small optimization to avoid running the regex too many times
    # this still increases the unit test execution time from 170s to 200s, should be able to accelerate it
    #
    # TODO(xusheng): performance optimizations to improve test execution time
    # https://github.com/mandiant/capa/issues/1610
    if name[0] in ["@", "_"]:
        match = re.match(r"^[@|_](.*?)(Stub)?(@\d+)?$", name)
        if match:
            return match.group(1)

    return name


def read_c_string(bv: BinaryView, offset: int, max_len: int) -> str:
    s = []
    while len(s) < max_len:
        try:
            c = bv.read(offset + count, 1)[0]
        except Exception:
            break

        if c == 0:
            break

        s.append(chr(c))

    return "".join(s)


Review comment (suggestion):
```suggestion
            c = bv.read(offset + len(s), 1)[0]
```

Code after:
    # https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170#FormatC
    # Possible variations for BaseThreadInitThunk:
    # @BaseThreadInitThunk@12
    # _BaseThreadInitThunk
    # _BaseThreadInitThunk@12
    # It is also possible for a function to have a `Stub` appended to its name:
    # _lstrlenWStub@4

    # A small optimization to avoid running the regex too many times
    # this still increases the unit test execution time from 170s to 200s, should be able to accelerate it
    #
    # TODO(xusheng): performance optimizations to improve test execution time
    # https://github.com/mandiant/capa/issues/1610
    if name[0] in ["@", "_"]:
        match = re.match(r"^[@|_](.*?)(Stub)?(@\d+)?$", name)
        if match:
            return match.group(1)

    return name


def read_c_string(bv: BinaryView, offset: int, max_len: int) -> str:
    s: List[str] = []
    while len(s) < max_len:
        try:
            c = bv.read(offset + len(s), 1)[0]
        except Exception:
            break

        if c == 0:
            break

        s.append(chr(c))

    return "".join(s)
Repository: mandiant/capa
File: capa/rules/cache.py (Python)

Code before:
        return None

    logger.debug("loading rule set from cache: %s", path)
    buf = path.read_bytes()

    try:
        cache = RuleCache.load(buf)
    except AssertionError:
        logger.debug("rule set cache is invalid: %s", path)
        # delete the cache that seems to be invalid.
        path.unlink()
        return None
    else:
        return cache.ruleset


def generate_rule_cache(rules_dir: Path, cache_dir: Path) -> bool:
    if not rules_dir.is_dir():
        logger.error(f"rules directory '{rules_dir}' does not exist")
        return False

    try:
        cache_dir.mkdir(parents=True, exist_ok=True)
        rules = capa.rules.get_rules([rules_dir], cache_dir)
    except (IOError, capa.rules.InvalidRule, capa.rules.InvalidRuleSet) as e:
        logger.error(f"{str(e)}")
        return False

    content = capa.rules.cache.get_ruleset_content(rules)
    id = capa.rules.cache.compute_cache_identifier(content)
    path = capa.rules.cache.get_cache_path(cache_dir, id)

    assert path.exists()
    logger.info("rules cache saved to: %s", path)

    return True


Review comment (suggestion):
```suggestion
        logger.error(f"{e:s}")
```

I think this is correct and more succinct, but maybe your way is more readable. 


Code after:
        return None

    logger.debug("loading rule set from cache: %s", path)
    buf = path.read_bytes()

    try:
        cache = RuleCache.load(buf)
    except AssertionError:
        logger.debug("rule set cache is invalid: %s", path)
        # delete the cache that seems to be invalid.
        path.unlink()
        return None
    else:
        return cache.ruleset


def generate_rule_cache(rules_dir: Path, cache_dir: Path) -> bool:
    if not rules_dir.is_dir():
        logger.error("rules directory %s does not exist", rules_dir)
        return False

    try:
        cache_dir.mkdir(parents=True, exist_ok=True)
        rules = capa.rules.get_rules([rules_dir], cache_dir)
    except (IOError, capa.rules.InvalidRule, capa.rules.InvalidRuleSet) as e:
        logger.error("%s", str(e))
        return False

    content = capa.rules.cache.get_ruleset_content(rules)
    id = capa.rules.cache.compute_cache_identifier(content)
    path = capa.rules.cache.get_cache_path(cache_dir, id)

    assert path.exists()
    logger.info("rules cache saved to: %s", path)

    return True
Repository: mandiant/capa
File: capa/ida/plugin/__init__.py (Python)

Code before:
# so, create this hook, wait for capa plugin to load, set the icon, and unhook.


class OnUpdatedActionsHook(ida_kernwin.UI_Hooks):
    """register a callback to be invoked each time the UI actions are updated"""

    def __init__(self, cb):
        super().__init__()
        self.cb = cb

    def updated_actions(self):
        if self.cb():
            # uninstall the callback once its run successfully
            self.unhook()


def install_icon():
    plugin_name = CapaExplorerPlugin.PLUGIN_NAME
    action_name = "Edit/Plugins/" + plugin_name

    if action_name not in ida_kernwin.get_registered_actions():
        # keep the hook registered
        return False

    # resource leak here. need to call `ida_kernwin.free_custom_icon`?
    # however, since we're not cycling this icon a lot, its probably ok.
    # expect to leak exactly one icon per application load.
    icon = ida_kernwin.load_custom_icon(data=ICON)

    ida_kernwin.update_action_icon(action_name, icon)

    # uninstall the hook
    return True


h = OnUpdatedActionsHook(install_icon)
h.hook()


Review comment (suggestion):
```suggestion
    # However, since we're not cycling this icon a lot, it's probably ok.
```

Code after:
# so, create this hook, wait for capa plugin to load, set the icon, and unhook.


class OnUpdatedActionsHook(ida_kernwin.UI_Hooks):
    """register a callback to be invoked each time the UI actions are updated"""

    def __init__(self, cb):
        super().__init__()
        self.cb = cb

    def updated_actions(self):
        if self.cb():
            # uninstall the callback once its run successfully
            self.unhook()


def install_icon():
    plugin_name = CapaExplorerPlugin.PLUGIN_NAME
    action_name = "Edit/Plugins/" + plugin_name

    if action_name not in ida_kernwin.get_registered_actions():
        # keep the hook registered
        return False

    # resource leak here. need to call `ida_kernwin.free_custom_icon`?
    # however, since we're not cycling this icon a lot, it's probably ok.
    # expect to leak exactly one icon per application load.
    icon = ida_kernwin.load_custom_icon(data=ICON)

    ida_kernwin.update_action_icon(action_name, icon)

    # uninstall the hook
    return True


h = OnUpdatedActionsHook(install_icon)
h.hook()
Repository: mandiant/capa
File: capa/helpers.py (Python)

Code before:
    # retrieve the latest modified cache file
    cache_files = list(cache_dir.glob("*.cache"))
    if not cache_files:
        logger.debug("no rule cache files found")
        return False

    latest_cache_file = max(cache_files, key=os.path.getmtime)
    cache_timestamp = os.path.getmtime(latest_cache_file)

    # these are the relevant rules code files that could conflict with using an outdated cache
    latest_rule_code_file = max([Path("capa/rules/__init__.py"), Path("capa/rules/cache.py")], key=os.path.getmtime)
    rule_code_timestamp = os.path.getmtime(latest_rule_code_file)

    if rule_code_timestamp > cache_timestamp:

        def ts_to_str(ts):
            return datetime.fromtimestamp(ts).strftime("%Y-%m-%d %H:%M:%S")

        logger.warning(
            "not using cache: latest rule code file %s (%s) is newer than the latest rule cache file %s (%s)",
            latest_rule_code_file,
            ts_to_str(rule_code_timestamp),
            latest_cache_file,
            ts_to_str(cache_timestamp),
        )
        logger.debug("delete the cache file manually to use rule caching again")
        return False

    logger.debug("no potentially outdated cache files found, cache can be used")
    return True


Review comment (performance):
```suggestion
        def ts_to_str(ts):
            return datetime.fromtimestamp(ts).strftime("%Y-%m-%d %H:%M:%S")

        logger.warning(
            "latest rule code file %s (%s) is newer than the latest rule cache file %s (%s)",
            latest_rule_code_file,
            ts_to_str(rule_code_timestamp),
            latest_cache_file,
            ts_to_str(cache_timestamp),
        )
```

Code after:
    # retrieve the latest modified cache file
    cache_files = list(cache_dir.glob("*.cache"))
    if not cache_files:
        logger.debug("no rule cache files found")
        return False

    latest_cache_file = max(cache_files, key=os.path.getmtime)
    cache_timestamp = os.path.getmtime(latest_cache_file)

    # these are the relevant rules code files that could conflict with using an outdated cache
    latest_rule_code_file = max([Path("capa/rules/__init__.py"), Path("capa/rules/cache.py")], key=os.path.getmtime)
    rule_code_timestamp = os.path.getmtime(latest_rule_code_file)

    if rule_code_timestamp > cache_timestamp:

        def ts_to_str(ts):
            return datetime.fromtimestamp(ts).strftime("%Y-%m-%d %H:%M:%S")

        logger.warning(
            "latest rule code file %s (%s) is newer than the latest rule cache file %s (%s)",
            latest_rule_code_file,
            ts_to_str(rule_code_timestamp),
            latest_cache_file,
            ts_to_str(cache_timestamp),
        )
        return False

    return True
Repository: mandiant/capa
File: capa/main.py (Python)

Code before:
BACKEND_AUTO = "auto"

E_MISSING_RULES = 10
E_MISSING_FILE = 11
E_INVALID_RULE = 12
E_CORRUPT_FILE = 13
E_FILE_LIMITATION = 14
E_INVALID_SIG = 15
E_INVALID_FILE_TYPE = 16
E_INVALID_FILE_ARCH = 17
E_INVALID_FILE_OS = 18
E_UNSUPPORTED_IDA_VERSION = 19
E_UNSUPPORTED_GHIDRA_VERSION = 20
E_MISSING_CAPE_STATIC_ANALYSIS = 21
E_MISSING_CAPE_DYNAMIC_ANALYSIS = 22
E_EMPTY_REPORT = 23
E_UNSUPPORTED_GHIDRA_EXECUTION_MODE = 24
E_INVALID_INPUT_FORMAT = 25
E_INVALID_FEATURE_EXTRACTOR = 26

logger = logging.getLogger("capa")


class FilterConfig(TypedDict, total=False):
    processes: set[int]
    functions: set[int]


@contextlib.contextmanager
def timing(msg: str):
    t0 = time.time()
    yield
    t1 = time.time()
    logger.debug("perf: %s: %0.2fs", msg, t1 - t0)


def set_vivisect_log_level(level):
    logging.getLogger("vivisect").setLevel(level)
    logging.getLogger("vivisect.base").setLevel(level)
    logging.getLogger("vivisect.impemu").setLevel(level)
    logging.getLogger("vtrace").setLevel(level)
    logging.getLogger("envi").setLevel(level)
    logging.getLogger("envi.codeflow").setLevel(level)
    logging.getLogger("Elf").setLevel(level)


def get_default_root() -> Path:
    """
    get the file system path to the default resources directory.
    under PyInstaller, this comes from _MEIPASS.
    under source, this is the root directory of the project.

Review comment (suggestion):
```suggestion
    processes: Set[int]
    functions: Set[int]
```

indexing `set` as a type hint was added in Python 3.9, whereas we still support 3.8 for a little longer.
https://docs.python.org/3/library/typing.html#typing.Set

Code after:
BACKEND_AUTO = "auto"

E_MISSING_RULES = 10
E_MISSING_FILE = 11
E_INVALID_RULE = 12
E_CORRUPT_FILE = 13
E_FILE_LIMITATION = 14
E_INVALID_SIG = 15
E_INVALID_FILE_TYPE = 16
E_INVALID_FILE_ARCH = 17
E_INVALID_FILE_OS = 18
E_UNSUPPORTED_IDA_VERSION = 19
E_UNSUPPORTED_GHIDRA_VERSION = 20
E_MISSING_CAPE_STATIC_ANALYSIS = 21
E_MISSING_CAPE_DYNAMIC_ANALYSIS = 22
E_EMPTY_REPORT = 23
E_UNSUPPORTED_GHIDRA_EXECUTION_MODE = 24
E_INVALID_INPUT_FORMAT = 25
E_INVALID_FEATURE_EXTRACTOR = 26

logger = logging.getLogger("capa")


class FilterConfig(TypedDict, total=False):
    processes: Set[int]
    functions: Set[int]


@contextlib.contextmanager
def timing(msg: str):
    t0 = time.time()
    yield
    t1 = time.time()
    logger.debug("perf: %s: %0.2fs", msg, t1 - t0)


def set_vivisect_log_level(level):
    logging.getLogger("vivisect").setLevel(level)
    logging.getLogger("vivisect.base").setLevel(level)
    logging.getLogger("vivisect.impemu").setLevel(level)
    logging.getLogger("vtrace").setLevel(level)
    logging.getLogger("envi").setLevel(level)
    logging.getLogger("envi.codeflow").setLevel(level)
    logging.getLogger("Elf").setLevel(level)


def get_default_root() -> Path:
    """
    get the file system path to the default resources directory.
    under PyInstaller, this comes from _MEIPASS.
    under source, this is the root directory of the project.
Repository: mandiant/capa
File: web/explorer/vite.config.js (JavaScript)

Code before:
import { defineConfig } from "vite";
import vue from "@vitejs/plugin-vue";
import { viteSingleFile } from "vite-plugin-singlefile";
import { fileURLToPath, URL } from "node:url";

// eslint-disable-next-line no-unused-vars
export default defineConfig(({ command, mode }) => {
    const isBundle = mode === "bundle";

    return {
        base: isBundle ? "/" : "/capa/explorer",
        plugins: isBundle ? [vue(), viteSingleFile()] : [vue()],
        resolve: {
            alias: {
                "@": fileURLToPath(new URL("src", import.meta.url)),
                "@testfiles": fileURLToPath(new URL("../../tests/data", import.meta.url))
            }
        }
    };
});


Review comment (bug):
```suggestion
        base: "./",
```

I think this might do it.
via: https://stackoverflow.com/a/69746868

Code after:
import { defineConfig } from "vite";
import vue from "@vitejs/plugin-vue";
import { viteSingleFile } from "vite-plugin-singlefile";
import { fileURLToPath, URL } from "node:url";

export default defineConfig(({ mode }) => {
    const isBundle = mode === "bundle";

    return {
        base: './',
        plugins: isBundle ? [vue(), viteSingleFile()] : [vue()],
        resolve: {
            alias: {
                "@": fileURLToPath(new URL("src", import.meta.url)),
                "@testfiles": fileURLToPath(new URL("../../tests/data", import.meta.url))
            }
        }
    };
});
Repository: mandiant/capa
File: capa/features/extractors/strings.py (Python)

Code before:
                return False

        else:
            # last loop, final chunk size is not aligned
            if not all(b == character for b in current_chunk):
                return False

    return True


def extract_ascii_strings(buf: bytes, n: int = 4) -> Iterator[String]:
    """
    Extract ASCII strings from the given binary data.

    Params:
      buf: the bytes from which to extract strings
      n: minimum string length
    """

    if not buf:
        return

    if (buf[0] in REPEATS) and buf_filled_with(buf, buf[0]):
        return

    r = None
    if n == 4:
        r = ASCII_RE_4
    else:
        reg = b"([%s]{%d,})" % (ASCII_BYTE, n)
        r = re.compile(reg)
    for match in r.finditer(buf):
        yield String(match.group().decode("ascii"), match.start())


def extract_unicode_strings(buf: bytes, n: int = 4) -> Iterator[String]:
    """
    Extract naive UTF-16 strings from the given binary data.

    Params:
      buf: the bytes from which to extract strings
      n: minimum string length
    """

    if not buf:
        return

    if (buf[0] in REPEATS) and buf_filled_with(buf, buf[0]):
        return

    if n == 4:

Review comment (refactor):
Similarly, check that lengh `n` is valid:
```py
def extract_ascii_strings(buf: bytes, n: int = 4) -> Iterator[String]:
  if n < 1:
    raise ValueError("minimum length must be positive")
```

Code after:
    for offset in range(0, len(buf), SLICE_SIZE):
        # bytes objects are immutable, so the slices share the underlying array,
        # and therefore this is cheap.
        current_chunk = buf[offset : offset + SLICE_SIZE]

        if len(current_chunk) == SLICE_SIZE:
            # chunk-aligned comparison

            if dupe_chunk != current_chunk:
                return False

        else:
            # last loop, final chunk size is not aligned
            if not all(b == character for b in current_chunk):
                return False

    return True


def extract_ascii_strings(buf: bytes, n: int = 4) -> Iterator[String]:
    """
    Extract ASCII strings from the given binary data.

    Params:
      buf: the bytes from which to extract strings
      n: minimum string length
    """

    if not buf:
        return

    if n < 1:
        raise ValueError("minimum string length must be positive")

    if (buf[0] in REPEATS) and buf_filled_with(buf, buf[0]):
        return

    r = None
    if n == 4:
        r = ASCII_RE_4
    else:
        reg = b"([%s]{%d,})" % (ASCII_BYTE, n)
        r = re.compile(reg)
    for match in r.finditer(buf):
        yield String(match.group().decode("ascii"), match.start())


def extract_unicode_strings(buf: bytes, n: int = 4) -> Iterator[String]:
    """
    Extract naive UTF-16 strings from the given binary data.
Repository: marktext/marktext
File: src/main/index.js (JavaScript)

Code before:
'use strict'

import { app, Menu } from 'electron'
import configureMenu, { dockMenu } from './menus'
import createWindow, { windows } from './createWindow'
// import { autoUpdater } from "electron-updater"

var openFilesCache = []

const onReady = () => {
  if (process.platform === 'win32' && process.argv.length >= 2) {
    openFilesCache = [process.argv[1]]
  }
  if (openFilesCache.length) {
    openFilesCache.forEach(path => createWindow(path))
    openFilesCache.length = 0 // empty the open file path cache
  } else {
    createWindow()
  }
  const menu = Menu.buildFromTemplate(configureMenu({ app }))
  Menu.setApplicationMenu(menu)
  if (process.platform === 'darwin') {
    // app.dock is only for macosx
    app.dock.setMenu(dockMenu)
  }
}

const openFile = (event, path) => {
  event.preventDefault()
  if (app.isReady()) {
    createWindow(path)
  } else {
    openFilesCache.push(path)

Review comment (suggestion):
Thanks for this PR.

`var` is not recommended, please use `let` to declare variables which will be changed latter.

Code after:
'use strict'

import { app, Menu } from 'electron'
import configureMenu, { dockMenu } from './menus'
import createWindow, { windows } from './createWindow'
// import { autoUpdater } from "electron-updater"

let openFilesCache = []

const onReady = () => {
  if (process.platform === 'win32' && process.argv.length >= 2) {
    openFilesCache = [process.argv[1]]
  }
  if (openFilesCache.length) {
    openFilesCache.forEach(path => createWindow(path))
    openFilesCache.length = 0 // empty the open file path cache
  } else {
    createWindow()
  }
  const menu = Menu.buildFromTemplate(configureMenu({ app }))
  Menu.setApplicationMenu(menu)
  if (process.platform === 'darwin') {
    // app.dock is only for macosx
    app.dock.setMenu(dockMenu)
  }
}

const openFile = (event, path) => {
  event.preventDefault()
  if (app.isReady()) {
    createWindow(path)
  } else {
    openFilesCache.push(path)
Repository: marktext/marktext
File: src/main/index.js (JavaScript)

Code before:
'use strict'

import { app, Menu } from 'electron'
import configureMenu, { dockMenu } from './menus'
import createWindow, { windows } from './createWindow'
// import { autoUpdater } from "electron-updater"

var openFilesCache = []

const onReady = () => {
  if (process.platform === 'win32' && process.argv.length >= 2) {
    openFilesCache = [process.argv[1]]
  }
  if (openFilesCache.length) {
    openFilesCache.forEach(path => createWindow(path))
    openFilesCache.length = 0 // empty the open file path cache
  } else {
    createWindow()
  }
  const menu = Menu.buildFromTemplate(configureMenu({ app }))
  Menu.setApplicationMenu(menu)
  if (process.platform === 'darwin') {
    // app.dock is only for macosx
    app.dock.setMenu(dockMenu)
  }
}

const openFile = (event, path) => {
  event.preventDefault()
  if (app.isReady()) {
    createWindow(path)
  } else {
    openFilesCache.push(path)

Review comment (suggestion):
You should use `let` instead of `var` to allow scope isolation. More info : https://medium.com/craft-academy/javascript-variables-should-you-use-let-var-or-const-394f7645c88f

Code after:
'use strict'

import { app, Menu } from 'electron'
import configureMenu, { dockMenu } from './menus'
import createWindow, { windows } from './createWindow'
// import { autoUpdater } from "electron-updater"

let openFilesCache = []

const onReady = () => {
  if (process.platform === 'win32' && process.argv.length >= 2) {
    openFilesCache = [process.argv[1]]
  }
  if (openFilesCache.length) {
    openFilesCache.forEach(path => createWindow(path))
    openFilesCache.length = 0 // empty the open file path cache
  } else {
    createWindow()
  }
  const menu = Menu.buildFromTemplate(configureMenu({ app }))
  Menu.setApplicationMenu(menu)
  if (process.platform === 'darwin') {
    // app.dock is only for macosx
    app.dock.setMenu(dockMenu)
  }
}

const openFile = (event, path) => {
  event.preventDefault()
  if (app.isReady()) {
    createWindow(path)
  } else {
    openFilesCache.push(path)