ronantakizawa/codereview-bench huggingface dataset:
Repository: hyprwm/Hyprland
File: src/helpers/sync/SyncTimeline.cpp (C++)

Code before:
#include "SyncTimeline.hpp"
#include "../../defines.hpp"
#include "../../managers/eventLoop/EventLoopManager.hpp"

#include <xf86drm.h>
#include <sys/eventfd.h>
using namespace Hyprutils::OS;

static bool checkDrmSyncobjTimelineSupport(int drmFD) {
    uint64_t cap = 0;
    int ret = drmGetCap(drmFD, DRM_CAP_SYNCOBJ_TIMELINE, &cap);
    return (ret == 0 && cap != 0);
}

SP<CSyncTimeline> CSyncTimeline::create(int drmFD_) {
    if (!checkDrmSyncobjTimelineSupport(drmFD_))
        return nullptr;

    auto timeline     = SP<CSyncTimeline>(new CSyncTimeline);
    timeline->m_drmFD = drmFD_;
    timeline->m_self  = timeline;

    if (drmSyncobjCreate(drmFD_, 0, &timeline->m_handle)) {
        Debug::log(ERR, "CSyncTimeline: failed to create a drm syncobj??");
        return nullptr;
    }

    return timeline;
}

SP<CSyncTimeline> CSyncTimeline::create(int drmFD_, CFileDescriptor&& drmSyncobjFD) {
    if (!checkDrmSyncobjTimelineSupport(drmFD_))
        return nullptr;

    auto timeline         = SP<CSyncTimeline>(new CSyncTimeline);
    timeline->m_drmFD     = drmFD_;
    timeline->m_syncobjFD = std::move(drmSyncobjFD);

Review comment (performance):
this should probably be static. It won't change, and I don't know how expensive drmGetCap is. Furthermore, you're duplicating this here and in protocolmanager. Maybe add a bool to CCompositor that is set in `::initServer`?

Code after:
#include "SyncTimeline.hpp"
#include "../../defines.hpp"
#include "../../managers/eventLoop/EventLoopManager.hpp"
#include "../../Compositor.hpp"

#include <xf86drm.h>
#include <sys/eventfd.h>
using namespace Hyprutils::OS;

SP<CSyncTimeline> CSyncTimeline::create(int drmFD_) {
    if (!g_pCompositor->supportsDrmSyncobjTimeline())
        return nullptr;

    auto timeline     = SP<CSyncTimeline>(new CSyncTimeline);
    timeline->m_drmFD = drmFD_;
    timeline->m_self  = timeline;

    if (drmSyncobjCreate(drmFD_, 0, &timeline->m_handle)) {
        Debug::log(ERR, "CSyncTimeline: failed to create a drm syncobj??");
        return nullptr;
    }

    return timeline;
}

SP<CSyncTimeline> CSyncTimeline::create(int drmFD_, CFileDescriptor&& drmSyncobjFD) {
    if (!g_pCompositor->supportsDrmSyncobjTimeline())
        return nullptr;

    auto timeline         = SP<CSyncTimeline>(new CSyncTimeline);
    timeline->m_drmFD     = drmFD_;
    timeline->m_syncobjFD = std::move(drmSyncobjFD);
    timeline->m_self      = timeline;

    if (drmSyncobjFDToHandle(drmFD_, timeline->m_syncobjFD.get(), &timeline->m_handle)) {
        Debug::log(ERR, "CSyncTimeline: failed to create a drm syncobj from fd??");
        return nullptr;
Repository: hyprwm/Hyprland
File: hyprtester/src/tests/main/window.cpp (C++)

Code before:
#include <cmath>
#include <thread>
#include <chrono>
#include <hyprutils/os/Process.hpp>
#include <hyprutils/memory/WeakPtr.hpp>

#include "../../shared.hpp"
#include "../../hyprctlCompat.hpp"
#include "../shared.hpp"
#include "tests.hpp"

static int  ret = 0;

static bool spawnKitty(const std::string& class_, int count) {
    NLog::log("{}Spawning {}", Colors::YELLOW, class_);
    if (!Tests::spawnKitty(class_)) {
        NLog::log("{}Error: {} did not spawn", Colors::RED, class_);
        return false;
    }
    NLog::log("{}Expecting {} window(s)", Colors::YELLOW, count);
    EXPECT(Tests::windowCount(), count);
    return true;
}

static bool test() {
    NLog::log("{}Testing windows", Colors::GREEN);

    // test on workspace "window"
    NLog::log("{}Switching to workspace `window`", Colors::YELLOW);
    getFromSocket("/dispatch workspace name:window");

    if (!spawnKitty("kitty_A", 1))
        return false;

    // check kitty properties. One kitty should take the entire screen, as this is smart gaps
    NLog::log("{}Expecting kitty_A to take up the whole screen", Colors::YELLOW);
    {
        auto str = getFromSocket("/clients");
        EXPECT(str.contains("at: 0,0"), true);
        EXPECT(str.contains("size: 1920,1080"), true);
        EXPECT(str.contains("fullscreen: 0"), true);
    }

    NLog::log("{}Testing window split ratios", Colors::YELLOW);
    {
        const double RATIO   = 1.25;

Review comment (suggestion):
Tests::spawnKitty already does this. The window count expect is redundant. You could just do EXPECT(Tests::spawnKitty(), true) iirc

the reason its there is because legacy. Window count tests can be dropped after kitty spawns

Code after:
#include <cmath>
#include <thread>
#include <chrono>
#include <hyprutils/os/Process.hpp>
#include <hyprutils/memory/WeakPtr.hpp>

#include "../../shared.hpp"
#include "../../hyprctlCompat.hpp"
#include "../shared.hpp"
#include "tests.hpp"

static int  ret = 0;

static bool spawnKitty(const std::string& class_) {
    NLog::log("{}Spawning {}", Colors::YELLOW, class_);
    if (!Tests::spawnKitty(class_)) {
        NLog::log("{}Error: {} did not spawn", Colors::RED, class_);
        return false;
    }
    return true;
}

static bool test() {
    NLog::log("{}Testing windows", Colors::GREEN);

    // test on workspace "window"
    NLog::log("{}Switching to workspace `window`", Colors::YELLOW);
    getFromSocket("/dispatch workspace name:window");

    if (!spawnKitty("kitty_A"))
        return false;

    // check kitty properties. One kitty should take the entire screen, as this is smart gaps
    NLog::log("{}Expecting kitty_A to take up the whole screen", Colors::YELLOW);
    {
        auto str = getFromSocket("/clients");
        EXPECT(str.contains("at: 0,0"), true);
        EXPECT(str.contains("size: 1920,1080"), true);
        EXPECT(str.contains("fullscreen: 0"), true);
    }

    NLog::log("{}Testing window split ratios", Colors::YELLOW);
    {
        const double RATIO   = 1.25;
        const double PERCENT = RATIO / 2.0 * 100.0;
        const int    GAPSIN  = 5;
Repository: hyprwm/Hyprland
File: src/protocols/types/SurfaceState.hpp (C++)

Code before:
#pragma once

#include "../../helpers/math/Math.hpp"
#include "../WaylandProtocol.hpp"
#include "./Buffer.hpp"
#include "helpers/time/Time.hpp"

class CTexture;
class CDRMSyncPointState;

struct SSurfaceStateFrameCB {
  public:
    SSurfaceStateFrameCB(SP<CWlCallback>&& resource_);
    ~SSurfaceStateFrameCB() noexcept = default;
    // disable copy
    SSurfaceStateFrameCB(const SSurfaceStateFrameCB&)            = delete;
    SSurfaceStateFrameCB& operator=(const SSurfaceStateFrameCB&) = delete;

    // allow move
    SSurfaceStateFrameCB(SSurfaceStateFrameCB&&) noexcept            = default;
    SSurfaceStateFrameCB& operator=(SSurfaceStateFrameCB&&) noexcept = default;

    bool                  good();
    void                  send(const Time::steady_tp& now);

  private:
    SP<CWlCallback> m_resource;
};

struct SSurfaceState {
    union {
        uint16_t all = 0;
        struct {
            bool buffer : 1;
            bool damage : 1;
            bool opaque : 1;

Review comment (suggestion):
do not move the callback resource class like this. Keep it as it was, you can add methods to it. Resources need to be uniform, this is inviting issues down the line.

There's nothing you added here that can't just be added to CWLCallbackResource

you can keep WP<> refs in the surface state

Code after:
#pragma once

#include "../../helpers/math/Math.hpp"
#include "../WaylandProtocol.hpp"
#include "./Buffer.hpp"

class CTexture;
class CDRMSyncPointState;
class CWLCallbackResource;

struct SSurfaceState {
    union {
        uint16_t all = 0;
        struct {
            bool buffer : 1;
            bool damage : 1;
            bool opaque : 1;
            bool input : 1;
            bool transform : 1;
            bool scale : 1;
            bool offset : 1;
            bool viewport : 1;
            bool acquire : 1;
            bool acked : 1;
            bool frame : 1;
        } bits;
    } updated;

    bool rejected = false;

    // initial values, copied from protocol text
    CHLBufferReference  buffer = {};                                          // The initial surface contents are void
    CRegion             damage, bufferDamage;                                 // The initial value for pending damage is empty
    CRegion             opaque;                                               // The initial value for an opaque region is empty
    CRegion             input     = CBox{{}, {INT32_MAX - 1, INT32_MAX - 1}}; // The initial value for an input region is infinite
    wl_output_transform transform = WL_OUTPUT_TRANSFORM_NORMAL;               // A newly created surface has its buffer transformation set to normal
Repository: hyprwm/Hyprland
File: src/helpers/CMType.cpp (C++)

Code before:
#include "CMType.hpp"
#include <string>
#include <unordered_map>
#include <format>

static std::unordered_map<std::string, NCMType::eCMType> const table = {{"auto", NCMType::CM_AUTO},   {"srgb", NCMType::CM_SRGB}, {"wide", NCMType::CM_WIDE},
                                                                        {"edid", NCMType::CM_EDID},   {"hdr", NCMType::CM_HDR},   {"hdredid", NCMType::CM_HDR_EDID},
                                                                        {"dcip3", NCMType::CM_DCIP3}, {"dp3", NCMType::CM_DP3},   {"adobe", NCMType::CM_ADOBE}};

NCMType::eCMType                                               NCMType::fromString(const std::string cmType) {
    auto it = table.find(cmType);
    if (it == table.end())
        throw std::invalid_argument(std::format("Unknown cm type {}", cmType));
    return it->second;
}

std::string NCMType::toString(eCMType cmType) {
    for (const auto& [key, value] : table) {
        if (value == cmType)
            return key;
    }
    throw std::invalid_argument(std::format("Unknown cm type enum value {}", static_cast<int>(cmType)));
}


Review comment (suggestion):
we don't use exceptions unless necessary cuz stl. Just return an empty string or use optional

Also, no `static_cast`, use `sc<>` from hyprutils

Code after:
#include "CMType.hpp"
#include <optional>
#include <string>
#include <unordered_map>

static std::unordered_map<std::string, NCMType::eCMType> const table = {{"auto", NCMType::CM_AUTO},   {"srgb", NCMType::CM_SRGB}, {"wide", NCMType::CM_WIDE},
                                                                        {"edid", NCMType::CM_EDID},   {"hdr", NCMType::CM_HDR},   {"hdredid", NCMType::CM_HDR_EDID},
                                                                        {"dcip3", NCMType::CM_DCIP3}, {"dp3", NCMType::CM_DP3},   {"adobe", NCMType::CM_ADOBE}};

std::optional<NCMType::eCMType>                                NCMType::fromString(const std::string cmType) {
    auto it = table.find(cmType);
    if (it == table.end())
        return std::nullopt;
    return it->second;
}

std::string NCMType::toString(eCMType cmType) {
    for (const auto& [key, value] : table) {
        if (value == cmType)
            return key;
    }
    return "";
}
Repository: hyprwm/Hyprland
File: hyprtester/src/tests/clients/child-window.cpp (C++)

Code before:
        return false;

    client.readBuf[bytesRead] = 0;
    std::string recieved      = std::string{client.readBuf.data()};
    recieved.pop_back();

    // wait for window to appear
    std::this_thread::sleep_for(std::chrono::milliseconds(2000));
    if (getFromSocket("/dispatch focuswindow class:child-test-child") != "ok") {
        NLog::log("{}Failed to focus child window", Colors::RED);
        return false;
    }


    return true;
}

static bool test() {
    SClient client;

    OK(getFromSocket("/keyword windowrule float, pin, class:child-test-parent"));
    if (!startClient(client))
        return false;

    createChild(client);
    EXPECT_CONTAINS(getFromSocket("/activewindow"), "pinned: 1");

    stopClient(client);
    NLog::log("{}Reloading config", Colors::YELLOW);
    OK(getFromSocket("/reload"));
    Tests::killAllWindows();
    EXPECT(Tests::windowCount(), 0);

    return !ret;
}

REGISTER_CLIENT_TEST_FN(test);

Review comment (suggestion):
you should check for pinned: 1 being in the string twice, once for the parent, once child :P `EXPECT_COUNT_STRING`

Code after:
    if ((size_t)write(client.writeFd.get(), cmd.c_str(), cmd.length()) != cmd.length())
        return false;

    if (!waitForWindow(client.proc, Tests::windowCount()))
        NLog::log("{}Child window took too long to open", Colors::RED);

    if (getFromSocket("/dispatch focuswindow class:child-test-child") != "ok") {
        NLog::log("{}Failed to focus child window", Colors::RED);
        return false;
    }

    return true;
}

static bool test() {
    SClient client;

    if (!startClient(client))
        return false;
    OK(getFromSocket("/dispatch setfloating class:child-test-parent"));
    OK(getFromSocket("/dispatch pin class:child-test-parent"));

    createChild(client);
    EXPECT(Tests::windowCount(), 2)
    EXPECT_COUNT_STRING(getFromSocket("/clients"), "pinned: 1", 2);

    stopClient(client);
    NLog::log("{}Reloading config", Colors::YELLOW);
    OK(getFromSocket("/reload"));
    Tests::killAllWindows();
    EXPECT(Tests::windowCount(), 0);

    return !ret;
}

REGISTER_CLIENT_TEST_FN(test);
Repository: ianstormtaylor/slate
File: packages/slate/src/transforms/node.ts (TypeScript)

Code before:
          const [commonNode] = commonNodeEntry as NodeEntry<Ancestor>
          const depth = commonPath.length + 1
          const wrapperPath = Path.next(lastPath.slice(0, depth))
          const wrapper = { ...element, children: [] }
          Transforms.insertNodes(editor, wrapper, { at: wrapperPath, voids })

          Transforms.moveNodes(editor, {
            at: range,
            match: n => commonNode.children.includes(n),
            to: wrapperPath.concat(0),
            voids,
          })
        }
      }
    })
  },
}

/**
 * Convert a range into a point by deleting it's content.
 */

const deleteRange = (editor: Editor, range: Range): Point | null => {
  if (Range.isCollapsed(range)) {
    return range.anchor
  } else {
    const [, end] = Range.edges(range)
    const pointRef = Editor.pointRef(editor, end)
    Transforms.delete(editor, { at: range })
    return pointRef.unref()
  }
}

const matchPath = (editor: Editor, path: Path): ((node: Node) => boolean) => {
  const [node] = Editor.node(editor, path)
  return n => n === node
}


Review comment (suggestion):
```suggestion
      // unwrapNode will call liftNode which does not support splitting the mode when nested.
      // If we do not reverse the order and call it from top to the bottom, it will remove all blocks 
      // that wrap target node. So we reverse the order.
      ).reverse()
```

I think a comment helps here. I'm happy to be told this is unnecessary. :)

Code after:
      const { mode = 'lowest', split = false, voids = false } = options
      let { at = editor.selection, match } = options

      if (!at) {
        return
      }

      if (match == null) {
        match = Path.isPath(at)
          ? matchPath(editor, at)
          : n => Editor.isBlock(editor, n)
      }

      if (Path.isPath(at)) {
        at = Editor.range(editor, at)
      }

      const rangeRef = Range.isRange(at) ? Editor.rangeRef(editor, at) : null
      const matches = Editor.nodes(editor, { at, match, mode, voids })
      const pathRefs = Array.from(
        matches,
        ([, p]) => Editor.pathRef(editor, p)
        // unwrapNode will call liftNode which does not support splitting the node when nested.
        // If we do not reverse the order and call it from top to the bottom, it will remove all blocks
        // that wrap target node. So we reverse the order.
      ).reverse()

      for (const pathRef of pathRefs) {
        const path = pathRef.unref()!
        const [node] = Editor.node(editor, path)
        let range = Editor.range(editor, path)

        if (split && rangeRef) {
          range = Range.intersection(rangeRef.current!, range)!
        }

        Transforms.liftNodes(editor, {
          at: range,
          match: n => Element.isAncestor(node) && node.children.includes(n),
          voids,
        })
      }

      if (rangeRef) {
        rangeRef.unref()
      }
    })
  },

  /**
   * Wrap the nodes at a location in a new container node, splitting the edges
Repository: ianstormtaylor/slate
File: packages/slate-react/src/hooks/use-isomorphic-layout-effect.ts (TypeScript)

Code before:
import { useLayoutEffect, useEffect } from 'react'

/**
 * Prevent warning on SSR by falling back to useEffect when window is not defined
 */
export const useIsomorphicLayoutEffect =
  typeof window !== 'undefined' && typeof Deno === 'undefined'
    ? useLayoutEffect
    : useEffect


Review comment (suggestion):
Checking for `Deno` shouldn't be needed. Try to align this conditional with the same checks React does internally:
https://github.com/facebook/react/blob/master/packages/shared/ExecutionEnvironment.js

Code after:
import { useLayoutEffect, useEffect } from 'react'
import { CAN_USE_DOM } from '../utils/environment'

/**
 * Prevent warning on SSR by falling back to useEffect when DOM isn't available
 */
export const useIsomorphicLayoutEffect = CAN_USE_DOM
  ? useLayoutEffect
  : useEffect
Repository: ianstormtaylor/slate
File: packages/slate-react/src/components/leaf.tsx (TypeScript)

Code before:
    parent,
    renderLeaf = (props: RenderLeafProps) => <DefaultLeaf {...props} />,
  } = props

  const placeholderRef = useRef<HTMLSpanElement | null>(null)

  useEffect(() => {
    const placeholderEl = placeholderRef?.current
    const editorEl = document.querySelector<HTMLDivElement>(
      '[data-slate-editor="true"]'
    )

    if (!placeholderEl || !editorEl) {
      return
    }

    editorEl.style.minHeight = `${placeholderEl.clientHeight}px`

    return () => {
      editorEl.style.minHeight = 'auto'
    }
  }, [placeholderRef])

  let children = (
    <String
      key={Math.random()}
      isLast={isLast}
      leaf={leaf}
      parent={parent}
      text={text}
    />
  )

  if (leaf[PLACEHOLDER_SYMBOL]) {
    children = (
      <React.Fragment>
        <span
          ref={placeholderRef}
          contentEditable={false}
          style={{
            pointerEvents: 'none',
            display: 'inline-block',
            width: '100%',
            maxWidth: '100%',
            whiteSpace: 'nowrap',
            opacity: '0.333',
            userSelect: 'none',
            fontStyle: 'normal',
            fontWeight: 'normal',
            textDecoration: 'none',
            position: 'absolute',

Review comment (suggestion):
Can you use a number key like `let key = 0` at the beginning of the file, then `key++` here instead? Makes it more standard and easier to augment in the future if we need.

Code after:
    leaf,
    isLast,
    text,
    parent,
    renderLeaf = (props: RenderLeafProps) => <DefaultLeaf {...props} />,
  } = props

  const placeholderRef = useRef<HTMLSpanElement | null>(null)

  useEffect(() => {
    const placeholderEl = placeholderRef?.current
    const editorEl = document.querySelector<HTMLDivElement>(
      '[data-slate-editor="true"]'
    )

    if (!placeholderEl || !editorEl) {
      return
    }

    editorEl.style.minHeight = `${placeholderEl.clientHeight}px`

    return () => {
      editorEl.style.minHeight = 'auto'
    }
  }, [placeholderRef])

  let children = (
    <String
      key={keyForString++}
      isLast={isLast}
      leaf={leaf}
      parent={parent}
      text={text}
    />
  )

  if (leaf[PLACEHOLDER_SYMBOL]) {
    children = (
      <React.Fragment>
        <span
          ref={placeholderRef}
          contentEditable={false}
          style={{
            pointerEvents: 'none',
            display: 'inline-block',
            width: '100%',
            maxWidth: '100%',
            whiteSpace: 'nowrap',
            opacity: '0.333',
            userSelect: 'none',
            fontStyle: 'normal',
Repository: ianstormtaylor/slate
File: packages/slate-react/src/components/editable.tsx (TypeScript)

Code before:
 * `RenderLeafProps` are passed to the `renderLeaf` handler.
 */

export interface RenderLeafProps {
  children: any
  leaf: Text
  text: Text
  attributes: {
    'data-slate-leaf': true
  }
}

/**
 * `EditableProps` are passed to the `<Editable>` component.
 */

export type EditableProps = {
  decorate?: (entry: NodeEntry) => Range[]
  onDOMBeforeInput?: (event: InputEvent) => void
  placeholder?: string
  readOnly?: boolean
  role?: string
  style?: React.CSSProperties
  renderElement?: (props: RenderElementProps) => JSX.Element
  renderLeaf?: (props: RenderLeafProps) => JSX.Element
  renderPlaceholder?: (props: typeof defaultPlaceholderProps) => JSX.Element
  as?: React.ElementType
} & React.TextareaHTMLAttributes<HTMLDivElement>

/**
 * Editable.
 */

export const Editable = (props: EditableProps) => {
  const {
    autoFocus,
    decorate = defaultDecorate,
    onDOMBeforeInput: propsOnDOMBeforeInput,
    placeholder,
    readOnly = false,
    renderElement,
    renderLeaf,
    renderPlaceholder,
    style = {},
    as: Component = 'div',
    ...attributes
  } = props
  const editor = useSlate()
  const ref = useRef<HTMLDivElement>(null)

  // Update internal state on each render.

Review comment (refactor):
Can you refactor this to be declared the same was as `RenderElementProps/RenderLeafProps` for consistency? So people can use those types in their own code if they need to.

Code after:
export type EditableProps = {
  decorate?: (entry: NodeEntry) => Range[]
  onDOMBeforeInput?: (event: InputEvent) => void
  placeholder?: string
  readOnly?: boolean
  role?: string
  style?: React.CSSProperties
  renderElement?: (props: RenderElementProps) => JSX.Element
  renderLeaf?: (props: RenderLeafProps) => JSX.Element
  renderPlaceholder?: (props: RenderPlaceholderProps) => JSX.Element
  as?: React.ElementType
} & React.TextareaHTMLAttributes<HTMLDivElement>

/**
 * Editable.
 */

export const Editable = (props: EditableProps) => {
  const {
    autoFocus,
    decorate = defaultDecorate,
    onDOMBeforeInput: propsOnDOMBeforeInput,
    placeholder,
    readOnly = false,
    renderElement,
    renderLeaf,
    renderPlaceholder = props => <DefaultPlaceholder {...props} />,
    style = {},
    as: Component = 'div',
    ...attributes
  } = props
  const editor = useSlate()
  const ref = useRef<HTMLDivElement>(null)

  // Update internal state on each render.
  IS_READ_ONLY.set(editor, readOnly)

  // Keep track of some state for the event handler logic.
  const state = useMemo(
    () => ({
      isComposing: false,
      isUpdatingSelection: false,
      latestElement: null as DOMElement | null,
    }),
    []
  )

  // Update element-related weak maps with the DOM element ref.
  useIsomorphicLayoutEffect(() => {
    let window
    if (ref.current && (window = getDefaultView(ref.current))) {