ronantakizawa/codereview-bench huggingface dataset:
Repository: shader-slang/slang
File: source/slang/slang-ast-print.cpp (C++)

Code before:
        }
    }
    else if (const auto parenExpr = as<ParenExpr>(expr))
    {
        sb << "(";
        if (parenExpr->base)
        {
            addExpr(parenExpr->base);
        }
        sb << ")";
    }
    else if (as<ThisExpr>(expr))
    {
        sb << "this";
    }
    else if (as<ReturnValExpr>(expr))
    {
        sb << "__return_val";
    }
    else if (as<SharedTypeExpr>(expr))
    {
        auto typeType = as<TypeType>(expr->type);
        if (typeType)
            sb << typeType->getType();
        else
            sb << as<TypeType>(expr->type);
    }
    else if (const auto letExpr = as<LetExpr>(expr))
    {
        sb << "let ";
        if (letExpr->decl)
        {
            _addDeclName(letExpr->decl);

            if (letExpr->decl->type.type)
            {
                sb << " : ";
                addType(letExpr->decl->type.type);
            }

            if (letExpr->decl->initExpr)
            {
                sb << " = ";
                addExpr(letExpr->decl->initExpr);
            }
        }

        if (letExpr->body)
        {
            sb << " in ";
            addExpr(letExpr->body);

Review comment (bug):
I am not sure if I am reading this right.
When `typeType` is nullptr, would `sb` always get nullptr here?
In other words, this can be simplified as following,
```
        auto typeType = as<TypeType>(expr->type);
        if (typeType)
            sb << typeType->getType();
        else
            sb << nullptr;
```
Or is this supposed to be more like the following?,
```
        auto typeType = as<TypeType>(expr->type);
        if (typeType->getType())
            sb << typeType->getType();
        else
            sb << typeType;
```


Code after:
        }
    }
    else if (const auto parenExpr = as<ParenExpr>(expr))
    {
        sb << "(";
        if (parenExpr->base)
        {
            addExpr(parenExpr->base);
        }
        sb << ")";
    }
    else if (as<ThisExpr>(expr))
    {
        sb << "this";
    }
    else if (as<ReturnValExpr>(expr))
    {
        sb << "__return_val";
    }
    else if (as<SharedTypeExpr>(expr))
    {
        auto typeType = as<TypeType>(expr->type);
        if (typeType)
            sb << typeType->getType();
        else
            sb << expr->type;
    }
    else if (const auto letExpr = as<LetExpr>(expr))
    {
        sb << "let ";
        if (letExpr->decl)
        {
            _addDeclName(letExpr->decl);

            if (letExpr->decl->type.type)
            {
                sb << " : ";
                addType(letExpr->decl->type.type);
            }

            if (letExpr->decl->initExpr)
            {
                sb << " = ";
                addExpr(letExpr->decl->initExpr);
            }
        }

        if (letExpr->body)
        {
            sb << " in ";
            addExpr(letExpr->body);
Repository: shader-slang/slang
File: source/slang/slang-ir-insts-stable-names.lua (Lua)

Code before:
	["GetTagFromSequentialID"] = 696,
	["GetSequentialIDFromTag"] = 697,
	["GetElementFromTag"] = 698,
	["GetDispatcher"] = 699,
	["GetSpecializedDispatcher"] = 700,
	["GetTagFromTaggedUnion"] = 701,
	["GetValueFromTaggedUnion"] = 702,
	["Type.UntaggedUnionType"] = 703,
	["Type.ElementOfSetType"] = 704,
	["MakeTaggedUnion"] = 705,
	["GetTypeTagFromTaggedUnion"] = 706,
	["GetTagOfElementInSet"] = 707,
	["UnboundedTypeElement"] = 708,
	["UnboundedFuncElement"] = 709,
	["UnboundedWitnessTableElement"] = 710,
	["UnboundedGenericElement"] = 711,
	["UninitializedTypeElement"] = 712,
	["UninitializedWitnessTableElement"] = 713,
	["NoneTypeElement"] = 714,
	["NoneWitnessTableElement"] = 715,
	["GetTagForSubSet"] = 716,
	["Decoration.DisallowSpecializationWithExistentialsDecoration"] = 717,
	["makeCoopMatrixFromScalar"] = 718,
	["getOptiXPayloadRegister"] = 719,
	["setOptiXPayloadRegister"] = 720,
	["Decoration.NoInline"] = 721
}


Review comment (suggestion):
It looks like we already have a decorator for no-inline.
Do we really need to add a new one?
```
["Decoration.noInline"] = 465,
```

Code after:
	["GetTagFromSequentialID"] = 696,
	["GetSequentialIDFromTag"] = 697,
	["GetElementFromTag"] = 698,
	["GetDispatcher"] = 699,
	["GetSpecializedDispatcher"] = 700,
	["GetTagFromTaggedUnion"] = 701,
	["GetValueFromTaggedUnion"] = 702,
	["Type.UntaggedUnionType"] = 703,
	["Type.ElementOfSetType"] = 704,
	["MakeTaggedUnion"] = 705,
	["GetTypeTagFromTaggedUnion"] = 706,
	["GetTagOfElementInSet"] = 707,
	["UnboundedTypeElement"] = 708,
	["UnboundedFuncElement"] = 709,
	["UnboundedWitnessTableElement"] = 710,
	["UnboundedGenericElement"] = 711,
	["UninitializedTypeElement"] = 712,
	["UninitializedWitnessTableElement"] = 713,
	["NoneTypeElement"] = 714,
	["NoneWitnessTableElement"] = 715,
	["GetTagForSubSet"] = 716,
	["Decoration.DisallowSpecializationWithExistentialsDecoration"] = 717,
	["makeCoopMatrixFromScalar"] = 718,
	["getOptiXPayloadRegister"] = 719,
	["setOptiXPayloadRegister"] = 720
}
Repository: shader-slang/slang
File: tools/slang-unit-test/unit-test-entrypoint-compile.cpp (C++)

Code before:
        }

        struct PS_INPUT 
        {
            float4 vPositionPs : SV_Position ; 
        }

        StructuredBuffer<float3> gBuffer;

        PS_INPUT vsMain(VS_INPUT i) 
        { 
            PS_INPUT o = {}; 
            float3 v = gBuffer[i.instanceId].xyz ; 
            o.vPositionPs = float4(v, 1.0f);
            return o ; 
        })";
    auto srcBlob = StringBlob::moveCreate(_Move(userSourceBody));
    auto module = session->loadModuleFromSource("m", "m.slang", srcBlob, diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(module != nullptr);

    ComPtr<slang::IEntryPoint> entryPoint;
    module->findAndCheckEntryPoint(
        "vsMain",
        SLANG_STAGE_VERTEX,
        entryPoint.writeRef(),
        diagnosticBlob.writeRef());

    ComPtr<slang::IComponentType> linkedProgram;
    entryPoint->link(linkedProgram.writeRef(), diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(linkedProgram != nullptr);

    ComPtr<slang::IBlob> code;
    linkedProgram->getEntryPointCode(0, 0, code.writeRef(), diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(code != nullptr);

    SLANG_CHECK_ABORT(code->getBufferSize() != 0);
}


Review comment (bug):
I have a question here.

Does `findAndCheckEntryPoint()` add the `EntryPointDecoration` to the function, "vsMain"?

I thought that the decoration requires an explicit notation of `[shader("XX")]` .

I am having a trouble understanding how the entry point "vsMain" here makes the function, `shouldProcessFunction()`, to return `true`:
```
    virtual bool shouldProcessFunction(IRFunc* func) override
    {
        if (!func->isDefinition())
            return false;
        if (func->findDecoration<IREntryPointDecoration>() != nullptr)
            return true;
        return false;
    }

Code after:
        }

        struct PS_INPUT 
        {
            float4 vPositionPs : SV_Position ; 
        }

        StructuredBuffer<float3> gBuffer;

        PS_INPUT vsMain(VS_INPUT i) 
        { 
            PS_INPUT o = {}; 
            float3 v = gBuffer[i.instanceId].xyz ; 
            o.vPositionPs = float4(v, 1.0f);
            return o ; 
        })";
    auto srcBlob = StringBlob::moveCreate(_Move(userSourceBody));
    auto module = session->loadModuleFromSource("m", "m.slang", srcBlob, diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(module != nullptr);

    ComPtr<slang::IEntryPoint> entryPoint;
    module->findAndCheckEntryPoint(
        "vsMain",
        SLANG_STAGE_VERTEX,
        entryPoint.writeRef(),
        diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(entryPoint != nullptr);

    ComPtr<slang::IComponentType> linkedProgram;
    entryPoint->link(linkedProgram.writeRef(), diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(linkedProgram != nullptr);

    ComPtr<slang::IBlob> code;
    linkedProgram->getEntryPointCode(0, 0, code.writeRef(), diagnosticBlob.writeRef());
    SLANG_CHECK_ABORT(code != nullptr);

    SLANG_CHECK_ABORT(code->getBufferSize() != 0);
}
Repository: shader-slang/slang
File: source/slang/slang-ir-cleanup-void.cpp (C++)

Code before:
        case kIROp_GetResultValue:
        case kIROp_Call:
        case kIROp_UpdateElement:
        case kIROp_GetTargetTupleElement:
            if (inst->getDataType()->getOp() == kIROp_VoidType)
            {
                IRBuilder builder(module);
                builder.setInsertBefore(inst);
                inst->replaceUsesWith(builder.getVoidValue());
            }
        }
    }

    void processModule()
    {
        addToWorkList(module->getModuleInst());

        while (workList.getCount() != 0)
        {
            IRInst* inst = workList.getLast();

            workList.removeLast();
            workListSet.remove(inst);

            if (inst != module->getModuleInst() && !inst->getParent())
                continue;

            processInst(inst);

            for (auto child = inst->getLastChild(); child; child = child->getPrevInst())
            {
                addToWorkList(child);
            }
        }

        for (auto inst : m_instsToRemove)
        {
            inst->removeAndDeallocate();
        }
    }
};

void cleanUpVoidType(IRModule* module)
{
    CleanUpVoidContext context(module);
    context.processModule();
}
} // namespace Slang


Review comment (suggestion):
I don't think we should need this check.
If needed, I think they shouldn't be in the workList in the first place; in other words, we should check right before calling `addToWorkList()` rather than check it after calling it.

Code after:
        case kIROp_Call:
        case kIROp_UpdateElement:
        case kIROp_GetTargetTupleElement:
            if (inst->getDataType()->getOp() == kIROp_VoidType)
            {
                IRBuilder builder(module);
                builder.setInsertBefore(inst);
                inst->replaceUsesWith(builder.getVoidValue());
            }
        }
    }

    void processModule()
    {
        addToWorkList(module->getModuleInst());

        while (workList.getCount() != 0)
        {
            IRInst* inst = workList.getLast();

            workList.removeLast();
            workListSet.remove(inst);

            processInst(inst);

            for (auto child = inst->getLastChild(); child; child = child->getPrevInst())
            {
                addToWorkList(child);
            }
        }

        for (auto inst : m_instsToRemove.getHashSet())
        {
            inst->removeAndDeallocate();
        }
    }
};

void cleanUpVoidType(IRModule* module)
{
    CleanUpVoidContext context(module);
    context.processModule();
}
} // namespace Slang
Repository: shader-slang/slang
File: tools/slang-test/test-context.h (C/C++)

Code before:
    std::mutex mutex;
    Slang::RefPtr<Slang::JSONRPCConnection> m_languageServerConnection;

    bool isRetry = false;
    std::mutex mutexFailedTests;
    Slang::List<Slang::RefPtr<FileTestInfo>> failedFileTests;
    Slang::List<Slang::String> failedUnitTests;

    Slang::IFileCheck* getFileCheck() { return m_fileCheck; };

protected:
    SlangResult _createJSONRPCConnection(Slang::RefPtr<Slang::JSONRPCConnection>& out);
    Slang::String _drainRPCConnectionStdErr();

    SlangResult locateFileCheck();

    struct SharedLibraryTool
    {
        Slang::ComPtr<ISlangSharedLibrary> m_sharedLibrary;
        InnerMainFunc m_func;
        CleanDeviceCacheFunc m_cleanDeviceCacheFunc;
    };

    Slang::List<Slang::RefPtr<Slang::JSONRPCConnection>> m_jsonRpcConnections;
    Slang::List<Slang::RefPtr<Slang::BufferedReadStream>> m_jsonRpcErrorStreams;
    Slang::List<Slang::RefPtr<Slang::Process>> m_jsonRpcProcesses;
    Slang::List<TestReporter*> m_reporters;
    Slang::List<TestRequirements*> m_testRequirements = nullptr;

    Slang::ComPtr<SlangSession> m_session;

    Slang::Dictionary<Slang::String, SharedLibraryTool> m_sharedLibTools;

    Slang::ComPtr<ISlangSharedLibrary> m_fileCheckLibrary;
    Slang::ComPtr<Slang::IFileCheck> m_fileCheck;
};

#endif // TEST_CONTEXT_H_INCLUDED


Review comment (suggestion):
The `process` value is already stored inside of m_jsonRpcConeections`.
It seems like `m_jsonRpcProcesses` is unnecessary duplication.

Code after:

    bool isRetry = false;
    std::mutex mutexFailedTests;
    Slang::List<Slang::RefPtr<FileTestInfo>> failedFileTests;
    Slang::List<Slang::String> failedUnitTests;

    Slang::IFileCheck* getFileCheck() { return m_fileCheck; };

protected:
    SlangResult _createJSONRPCConnection(Slang::RefPtr<Slang::JSONRPCConnection>& out);

    SlangResult locateFileCheck();

    struct SharedLibraryTool
    {
        Slang::ComPtr<ISlangSharedLibrary> m_sharedLibrary;
        InnerMainFunc m_func;
        CleanDeviceCacheFunc m_cleanDeviceCacheFunc;
    };

    Slang::List<Slang::RefPtr<Slang::JSONRPCConnection>> m_jsonRpcConnections;
    Slang::List<TestReporter*> m_reporters;
    Slang::List<TestRequirements*> m_testRequirements = nullptr;

    Slang::ComPtr<SlangSession> m_session;

    Slang::Dictionary<Slang::String, SharedLibraryTool> m_sharedLibTools;

    Slang::ComPtr<ISlangSharedLibrary> m_fileCheckLibrary;
    Slang::ComPtr<Slang::IFileCheck> m_fileCheck;
};

#endif // TEST_CONTEXT_H_INCLUDED
Repository: slidevjs/slidev
File: packages/slidev/node/cli.ts (TypeScript)

Code before:
        fullname: 'open',
        action() {
          openBrowser(`http://localhost:${port}`)
        },
      },
      {
        name: 'e',
        fullname: 'edit',
        action() {
          exec(`code "${entry}"`)
        },
      },
    ]

    function bindShortcut() {
      process.stdin.resume()
      process.stdin.setEncoding('utf8')
      process.stdin.on('data', (data) => {
        const str = data.toString().trim().toLowerCase()
        const [sh] = SHORTCUTS.filter(item => item.name === str)
        if (sh) {
          try {
            sh.action()
          }
          catch (err) {
            console.error(`error occurred while executing shortcut ${sh.fullname}: ${err}`)
          }
        }
      })
    }

    initServer()
    bindShortcut()
  },
)

cli.command(
  'build [entry]',
  'Build hostable SPA',
  args => commonOptions(args)
    .option('watch', {
      alias: 'w',
      default: false,
      describe: 'build watch',
    })
    .option('out', {
      alias: 'o',
      type: 'string',
      default: 'dist',
      describe: 'output dir',
    })

Review comment (bug):
```suggestion
            console.error(`Failed to execute shortcut ${sh.fullname}`, err)
```

Code after:
        fullname: 'open',
        action() {
          openBrowser(`http://localhost:${port}`)
        },
      },
      {
        name: 'e',
        fullname: 'edit',
        action() {
          exec(`code "${entry}"`)
        },
      },
    ]

    function bindShortcut() {
      process.stdin.resume()
      process.stdin.setEncoding('utf8')
      process.stdin.on('data', (data) => {
        const str = data.toString().trim().toLowerCase()
        const [sh] = SHORTCUTS.filter(item => item.name === str)
        if (sh) {
          try {
            sh.action()
          }
          catch (err) {
            console.error(`Failed to execute shortcut ${sh.fullname}`, err)
          }
        }
      })
    }

    initServer()
    bindShortcut()
  },
)

cli.command(
  'build [entry]',
  'Build hostable SPA',
  args => commonOptions(args)
    .option('watch', {
      alias: 'w',
      default: false,
      describe: 'build watch',
    })
    .option('out', {
      alias: 'o',
      type: 'string',
      default: 'dist',
      describe: 'output dir',
    })
Repository: slidevjs/slidev
File: packages/types/src/config.ts (TypeScript)

Code before:
   * @default true
   */
  enabled?: boolean | 'dev' | 'build'

  /**
   * Only allow drawing from presenter mode
   *
   * @default false
   */
  presenterOnly?: boolean

  /**
   * Sync drawing for all instances
   *
   * @default true
   */
  syncAll?: boolean
}

export interface PresenterOptions {
  /**
   * Restrict access to presenter mode
   *
   * @default true
   */
  isPrivate?: boolean
  /**
   * Remote access key for the presenter mode
   *
   * @default true
   */
  token?: string
}

export interface ResolvedFontOptions {
  sans: string[]
  mono: string[]
  serif: string[]
  weights: string[]
  italic: boolean
  provider: 'none' | 'google'
  webfonts: string[]
  local: string[]
}

export interface ResolvedDrawingsOptions {
  persist: string | false
  enabled: boolean | 'dev' | 'build'
  presenterOnly: boolean
  syncAll: boolean
}

Review comment (security):
Maybe just `private`? And I would prefer to have it default to `false` as you already need to explicitly pass `--remote` flag.

Or more over, maybe we could unify this with `token` to a single entity called `password`. And we could have the cli to be `--remote mypassowrd`.

By that, we could have a basic auth header to prompt for passing the password when visiting the route. (Or just have it in the UI)

Code after:

  /**
   * @default true
   */
  enabled?: boolean | 'dev' | 'build'

  /**
   * Only allow drawing from presenter mode
   *
   * @default false
   */
  presenterOnly?: boolean

  /**
   * Sync drawing for all instances
   *
   * @default true
   */
  syncAll?: boolean
}

export interface ResolvedFontOptions {
  sans: string[]
  mono: string[]
  serif: string[]
  weights: string[]
  italic: boolean
  provider: 'none' | 'google'
  webfonts: string[]
  local: string[]
}

export interface ResolvedDrawingsOptions {
  persist: string | false
  enabled: boolean | 'dev' | 'build'
  presenterOnly: boolean
  syncAll: boolean
}
Repository: slidevjs/slidev
File: packages/types/src/config.ts (TypeScript)

Code before:
   */
  favicon: string
  /**
   * Options for drawings
   *
   * @default {}
   */
  drawings: ResolvedDrawingsOptions
  /**
   * URL of PlantUML server used to render diagrams
   *
   * @default https://www.plantuml.com/plantuml
   */
  plantUmlServer: string
  /**
   * Enable slides recording
   *
   * @default 'dev'
   */
  record: boolean | 'dev' | 'build'
  /**
   * Remote password
   *
   * @default undefined
   */
  remote?: string
}

export interface FontOptions {
  /**
   * Sans serif fonts (default fonts for most text)
   */
  sans?: string | string[]
  /**
   * Serif fonts
   */
  serif?: string | string[]
  /**
   * Monospace fonts, for code blocks and etc.
   */
  mono?: string | string[]
  /**
   * Load webfonts for custom CSS (does not apply anywhere by default)
   */
  custom?: string | string[]
  /**
   * Weights for fonts
   *
   * @default [200, 400, 600]
   */
  weights?: string | (string | number)[]

Review comment (suggestion):
```suggestion
  /**
   * Expose the server to inbound requests (listen to `0.0.0.0`)
   *
   * Pass a string to set the password for accessing presenter mode.
   *
   * @default false
   */
  remote?: string | boolean
```

Code after:
   */
  favicon: string
  /**
   * Options for drawings
   *
   * @default {}
   */
  drawings: ResolvedDrawingsOptions
  /**
   * URL of PlantUML server used to render diagrams
   *
   * @default https://www.plantuml.com/plantuml
   */
  plantUmlServer: string
  /**
   * Enable slides recording
   *
   * @default 'dev'
   */
  record: boolean | 'dev' | 'build'
  /**
   * Expose the server to inbound requests (listen to `0.0.0.0`)
   *
   * Pass a string to set the password for accessing presenter mode.
   *
   * @default false
   */
  remote?: string | boolean
}

export interface FontOptions {
  /**
   * Sans serif fonts (default fonts for most text)
   */
  sans?: string | string[]
  /**
   * Serif fonts
   */
  serif?: string | string[]
  /**
   * Monospace fonts, for code blocks and etc.
   */
  mono?: string | string[]
  /**
   * Load webfonts for custom CSS (does not apply anywhere by default)
   */
  custom?: string | string[]
  /**
   * Weights for fonts
   *
   * @default [200, 400, 600]
Repository: slidevjs/slidev
File: packages/types/src/setups.ts (TypeScript)

Code before:
  router: Router
}

export interface ShikiDarkModeThemes {
  dark: IThemeRegistration
  light: IThemeRegistration
}

export interface ShikiOptions {
  theme?: IThemeRegistration | ShikiDarkModeThemes
  langs?: ILanguageRegistration[]
  highlighter?: ShikiHighlighter
}

export interface MonacoSetupReturn {
  theme?: {
    light?: string
    dark?: string
  }
}

export type MermaidOptions = (typeof mermaid.initialize) extends (a: infer A) => any ? A : never

export interface NavOperations {
  firstSlide: () => void
  lastSlide: () => void
  next: () => void
  prev: () => Promise<void>
  nextSlide: () => void
  prevSlide: () => Promise<void>
  go: (index: number) => void
  downloadPDF: () => Promise<void>
  toggleDark: () => void
  toggleOverview: () => void
  toggleDrawing: () => void
  escapeOverview: () => void
  showGotoDialog: () => void
}

export interface ShortcutOptions {
  key: string | Ref<boolean>
  fn?: () => void
  autoRepeat?: boolean
}

// node side
export type ShikiSetup = (shiki: typeof Shiki) => Awaitable<ShikiOptions | undefined>
export type KatexSetup = () => Awaitable<Partial<KatexOptions> | undefined>
export type WindiSetup = () => Awaitable<Partial<WindiCssOptions> | undefined>

// client side

Review comment (suggestion):
```suggestion
  goFirst: () => void
  goLast: () => void
```

Code after:
  app: App
  router: Router
}

export interface ShikiDarkModeThemes {
  dark: IThemeRegistration
  light: IThemeRegistration
}

export interface ShikiOptions {
  theme?: IThemeRegistration | ShikiDarkModeThemes
  langs?: ILanguageRegistration[]
  highlighter?: ShikiHighlighter
}

export interface MonacoSetupReturn {
  theme?: {
    light?: string
    dark?: string
  }
}

export type MermaidOptions = (typeof mermaid.initialize) extends (a: infer A) => any ? A : never

export interface NavOperations {
  next: () => void
  prev: () => Promise<void>
  nextSlide: () => void
  prevSlide: () => Promise<void>
  go: (index: number) => void
  goFirst: () => void
  goLast: () => void
  downloadPDF: () => Promise<void>
  toggleDark: () => void
  toggleOverview: () => void
  toggleDrawing: () => void
  escapeOverview: () => void
  showGotoDialog: () => void
}

export interface ShortcutOptions {
  key: string | Ref<boolean>
  fn?: () => void
  autoRepeat?: boolean
}

// node side
export type ShikiSetup = (shiki: typeof Shiki) => Awaitable<ShikiOptions | undefined>
export type KatexSetup = () => Awaitable<Partial<KatexOptions> | undefined>
export type WindiSetup = () => Awaitable<Partial<WindiCssOptions> | undefined>
export type UnoSetup = () => Awaitable<Partial<UnoCssConfig> | undefined>