ronantakizawa/codereview-bench huggingface dataset:
Repository: serde-rs/serde
File: test_suite/tests/test_annotations.rs (Rust)

Code before:

#[test]
fn test_flatten_unit() {
    #[derive(Debug, PartialEq, Serialize, Deserialize)]
    struct Response<T> {
        #[serde(flatten)]
        data: T,
        status: usize,
    }

    assert_tokens(
        &Response {
            data: (),
            status: 0,
        },
        &[
            Token::Map { len: None },
            Token::Str("status"),
            Token::U64(0),
            Token::MapEnd,
        ],
    );
}

#[test]
fn test_flatten_unsupported_type() {
    #[derive(Debug, PartialEq, Serialize, Deserialize)]
    struct Outer {
        outer: String,
        #[serde(flatten)]
        inner: String,
    }

    assert_ser_tokens_error(
        &Outer {
            outer: "foo".into(),
            inner: "bar".into(),
        },
        &[
            Token::Map { len: None },
            Token::Str("outer"),
            Token::Str("foo"),
        ],
        "can only flatten structs and maps (got a string)",
    );
    assert_de_tokens_error::<Outer>(
        &[
            Token::Map { len: None },
            Token::Str("outer"),
            Token::Str("foo"),
            Token::Str("a"),

Review comment (suggestion):
It would be great if new flatten tests were added to the `flatten` module in the end of this file. Just my opinion

Code after:

#[test]
fn test_flatten_unit() {
    #[derive(Debug, PartialEq, Serialize, Deserialize)]
    struct Response<T> {
        #[serde(flatten)]
        data: T,
        status: usize,
    }

    assert_tokens(
        &Response {
            data: (),
            status: 0,
        },
        &[
            Token::Map { len: None },
            Token::Str("status"),
            Token::U64(0),
            Token::MapEnd,
        ],
    );
}

#[test]
fn test_flatten_unit_struct() {
    #[derive(Debug, PartialEq, Serialize, Deserialize)]
    struct Response<T> {
        #[serde(flatten)]
        data: T,
        status: usize,
    }

    #[derive(Debug, PartialEq, Serialize, Deserialize)]
    struct Unit;

    assert_tokens(
        &Response {
            data: Unit,
            status: 0,
        },
        &[
            Token::Map { len: None },
            Token::Str("status"),
            Token::U64(0),
            Token::MapEnd,
        ],
    );
}

#[test]
Repository: shader-slang/slang
File: source/slang/slang-emit-c-like.cpp (C++)

Code before:
    }
    else
    {
        // The common case is that what we
        // have is just an ordinary function,
        // and we can emit it as such.
        //
        emitSimpleFunc(func);
    }
    if (shouldCloseExternCBlock)
    {
        m_writer->emit("}\n");
    }
}

void CLikeSourceEmitter::emitFuncDecorationsImpl(IRFunc* func)
{
    for(auto decoration : func->getDecorations())
    {
        emitFuncDecorationImpl(decoration);
    }
}

void CLikeSourceEmitter::emitStruct(IRStructType* structType)
{
    if (shouldEmitOnlyHeader() && !structType->findDecoration<IRExternCppDecoration>())
    {
        return;
    }

    ensureTypePrelude(structType);

    // If the selected `struct` type is actually an intrinsic
    // on our target, then we don't want to emit anything at all.
    if(isTargetIntrinsic(structType))
    {
        return;
    }

    m_writer->emit("struct ");

    emitPostKeywordTypeAttributes(structType);

    m_writer->emit(getName(structType));

    emitStructDeclarationsBlock(structType, false);
    m_writer->emit(";\n\n");
}

void CLikeSourceEmitter::emitStructDeclarationsBlock(IRStructType* structType, bool allowOffsetLayout)
{

Review comment (suggestion):
Instead of skipping here, we should just make sure we never generate the work to emit the struct from the beginning. See `CLikeSourceEmitter::computeEmitActions` where we are trying to figure out what insts needs to be emitted. We should just make sure the irrelavent structs are not added. Also if you have:
```
struct MyType {}

[HeaderExport]
void myFunc(MyType t);
```

That should result `MyType` to be emitted automatically. By carefully handling `computeEmitActions` we should be able to handle this gracefully.

Code after:
        // force uppercase.
        //
        String name = semanticAttr->getName();
        name = name.toUpper();

        m_writer->emit(" : ");
        m_writer->emit(name);
        if (auto index = semanticAttr->getIndex())
        {
            m_writer->emit(index);
        }
    }
}

void CLikeSourceEmitter::emitSemanticsPrefix(IRInst* inst)
{
    emitSemanticsPrefixImpl(inst);
}

void CLikeSourceEmitter::emitSemantics(IRInst* inst, bool allowOffsetLayout)
{
    emitSemanticsImpl(inst, allowOffsetLayout);
}

void CLikeSourceEmitter::emitDecorationLayoutSemantics(
    IRInst* inst,
    char const* uniformSemanticSpelling)
{
    emitLayoutSemanticsImpl(inst, uniformSemanticSpelling, EmitLayoutSemanticOption::kPreType);
}

void CLikeSourceEmitter::emitLayoutSemantics(IRInst* inst, char const* uniformSemanticSpelling)
{
    emitLayoutSemanticsImpl(inst, uniformSemanticSpelling, EmitLayoutSemanticOption::kPostType);
}

void CLikeSourceEmitter::emitSwitchCaseSelectorsImpl(
    const SwitchRegion::Case* currentCase,
    bool isDefault)
{
    for (auto caseVal : currentCase->values)
    {
        m_writer->emit("case ");
        emitOperand(caseVal, getInfo(EmitOp::General));
        m_writer->emit(":\n");
    }
    if (isDefault)
    {
        m_writer->emit("default:\n");
    }
}
Repository: shader-slang/slang
File: source/slang/slang-ir-constexpr.cpp (C++)

Code before:
            return true;
    }

    return false;
}

bool isConstExpr(IRInst* value)
{
    // Certain IR value ops are implicitly `constexpr`
    //
    // TODO: should we just go ahead and make that explicit
    // in the type system?
    switch (value->getOp())
    {
    case kIROp_IntLit:
    case kIROp_FloatLit:
    case kIROp_BoolLit:
    case kIROp_Func:
    case kIROp_StructKey:
    case kIROp_WitnessTable:
    case kIROp_Generic:
        return true;
    //case kIROp_Param:
    //    if (isGenericParam(value))
    //        return true;
    //    break;
    default:
        break;
    }

    if (isConstExpr(value->getFullType()))
        return true;

    return false;
}

bool opCanBeConstExpr(IROp op)
{
    switch (op)
    {
    case kIROp_IntLit:
    case kIROp_FloatLit:
    case kIROp_BoolLit:
    case kIROp_Param:
    case kIROp_Add:
    case kIROp_Sub:
    case kIROp_Mul:
    case kIROp_Div:
    case kIROp_IRem:
    case kIROp_FRem:
    case kIROp_Neg:

Review comment (bug):
with this, `tests\language-feature\types\conditional-varying.slang` failed because `IR_intCast(bool)`, where the bool is a generic param, is marked constexpr, but its operand is not

Code after:
            return true;
    }

    return false;
}

bool isConstExpr(IRInst* value)
{
    // Certain IR value ops are implicitly `constexpr`
    //
    // TODO: should we just go ahead and make that explicit
    // in the type system?
    switch (value->getOp())
    {
    case kIROp_IntLit:
    case kIROp_FloatLit:
    case kIROp_BoolLit:
    case kIROp_Func:
    case kIROp_StructKey:
    case kIROp_WitnessTable:
    case kIROp_Generic:
    case kIROp_GlobalConstant:
        return true;
    case kIROp_Param:
        if (isGenericParam(value))
            return true;
        break;
    default:
        break;
    }

    if (isConstExpr(value->getFullType()))
        return true;

    return false;
}

bool opCanBeConstExpr(IROp op)
{
    switch (op)
    {
    case kIROp_IntLit:
    case kIROp_FloatLit:
    case kIROp_BoolLit:
    case kIROp_Param:
    case kIROp_Add:
    case kIROp_Sub:
    case kIROp_Mul:
    case kIROp_Div:
    case kIROp_IRem:
    case kIROp_FRem:
Repository: shader-slang/slang
File: source/core/slang-io.cpp (C++)

Code before:
#include <unistd.h>
// For Path::find
#include <dirent.h>
#include <fnmatch.h>
#include <ftw.h> // for nftw
#include <sys/file.h>
#include <sys/stat.h>
#endif

#if SLANG_APPLE_FAMILY
#include <mach-o/dyld.h>
#endif

#include <filesystem>
#include <limits.h> /* PATH_MAX */
#include <stdio.h>
#include <stdlib.h>

namespace Slang
{

/* static */ SlangResult File::remove(const String& fileName)
{
#ifdef _WIN32
    // Convert to wide string for Unicode support
    int wideLen = MultiByteToWideChar(CP_UTF8, 0, fileName.getBuffer(), -1, nullptr, 0);
    if (wideLen == 0)
        return SLANG_FAIL;
    
    wchar_t* wideFileName = (wchar_t*)_alloca(wideLen * sizeof(wchar_t));
    if (MultiByteToWideChar(CP_UTF8, 0, fileName.getBuffer(), -1, wideFileName, wideLen) == 0)
        return SLANG_FAIL;

    // https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-deletefilew
    if (DeleteFileW(wideFileName))
    {
        return SLANG_OK;
    }
    return SLANG_FAIL;
#else
    // https://linux.die.net/man/3/remove
    if (::remove(fileName.getBuffer()) == 0)
    {
        return SLANG_OK;
    }
    return SLANG_FAIL;
#endif
}


#ifdef _WIN32

Review comment (suggestion):
use `fileName.toWString()` to get `wchar_t*`, don't call windows API directly to do the translation.

Code after:
#include <unistd.h>
// For Path::find
#include <dirent.h>
#include <fnmatch.h>
#include <ftw.h> // for nftw
#include <sys/file.h>
#include <sys/stat.h>
#endif

#if SLANG_APPLE_FAMILY
#include <mach-o/dyld.h>
#endif

#include <filesystem>
#include <limits.h> /* PATH_MAX */
#include <stdio.h>
#include <stdlib.h>

namespace Slang
{

/* static */ SlangResult File::remove(const String& fileName)
{
#ifdef _WIN32

    // https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-deletefilew
    if (DeleteFileW(fileName.toWString()))
    {
        return SLANG_OK;
    }
    return SLANG_FAIL;
#else
    // https://linux.die.net/man/3/remove
    if (::remove(fileName.getBuffer()) == 0)
    {
        return SLANG_OK;
    }
    return SLANG_FAIL;
#endif
}


#ifdef _WIN32
/* static */ SlangResult File::generateTemporary(
    const UnownedStringSlice& inPrefix,
    Slang::String& outFileName)
{
    // https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-using-a-temporary-file

    String tempPath;
    {
Repository: shader-slang/slang
File: tools/render-test/slang-test-device-cache.h (C/C++)

Code before:
public:
    struct DeviceCacheKey
    {
        rhi::DeviceType deviceType;
        bool enableValidation;
        bool enableRayTracingValidation;
        std::string profileName;
        std::vector<std::string> requiredFeatures;

        bool operator==(const DeviceCacheKey& other) const;
    };

    struct DeviceCacheKeyHash
    {
        std::size_t operator()(const DeviceCacheKey& key) const;
    };

    struct CachedDevice
    {
        Slang::ComPtr<rhi::IDevice> device;
        uint64_t creationOrder;

        CachedDevice();
    };

private:
    static constexpr int MAX_CACHED_DEVICES = 10;

    // Use function-local statics to control destruction order (Meyer's singleton pattern)
    static std::mutex& getMutex();
    static std::unordered_map<DeviceCacheKey, CachedDevice, DeviceCacheKeyHash>& getDeviceCache();
    static uint64_t& getNextCreationOrder();

    static void evictOldestDeviceIfNeeded();

public:
    static Slang::ComPtr<rhi::IDevice> acquireDevice(const rhi::DeviceDesc& desc);
    static void cleanCache();
};

// RAII wrapper for cached devices to ensure proper cleanup
class CachedDeviceWrapper
{
private:
    Slang::ComPtr<rhi::IDevice> m_device;

public:
    CachedDeviceWrapper() = default;

    CachedDeviceWrapper(Slang::ComPtr<rhi::IDevice> device)
        : m_device(device)

Review comment (bug):
Feel free to ignore this comment, but we may want to set the value from the command-line argument for a debugging purpose.

Code after:
    struct DeviceCacheKey
    {
        rhi::DeviceType deviceType;
        bool enableValidation;
        bool enableRayTracingValidation;
        std::string profileName;
        std::vector<std::string> requiredFeatures;

        bool operator==(const DeviceCacheKey& other) const;
    };

    struct DeviceCacheKeyHash
    {
        std::size_t operator()(const DeviceCacheKey& key) const;
    };

    struct CachedDevice
    {
        Slang::ComPtr<rhi::IDevice> device;
        uint64_t creationOrder;

        CachedDevice();
    };

private:
    static constexpr int MAX_CACHED_DEVICES = 10;

    // Use function-local statics to control destruction order (Meyer's singleton pattern)
    static std::mutex& getMutex();
    static std::unordered_map<DeviceCacheKey, CachedDevice, DeviceCacheKeyHash>& getDeviceCache();
    static uint64_t& getNextCreationOrder();

    static void evictOldestDeviceIfNeeded();

public:
    static SlangResult acquireDevice(const rhi::DeviceDesc& desc, rhi::IDevice** outDevice);
    static void cleanCache();
};

// RAII wrapper for cached devices to ensure proper cleanup
class CachedDeviceWrapper
{
private:
    Slang::ComPtr<rhi::IDevice> m_device;

public:
    CachedDeviceWrapper() = default;

    CachedDeviceWrapper(Slang::ComPtr<rhi::IDevice> device)
        : m_device(device)
    {
Repository: shader-slang/slang
File: source/slang/slang-ir-util.cpp (C++)

Code before:
        break;
    case kIROp_CastPtrToInt:
    case kIROp_Reinterpret:
    case kIROp_BitCast:
        {
            // If we are trying to cast an address to something else, return true.
            if (isPtrLikeOrHandleType(inst->getOperand(0)->getDataType()) &&
                canAddressesPotentiallyAlias(func, inst->getOperand(0), addr))
                return true;
            else if (!isValueType(inst->getOperand(0)->getDataType()))
            {
                // This is some unknown handle type, we assume it can have any side effects.
                return true;
            }
        }
        break;
    default:
        // Default behavior is that any insts that have side effect may affect `addr`.
        if (inst->mightHaveSideEffects())
            return true;
        break;
    }
    return false;
}

IRInst* getUnitPoisionVal(IRBuilder builder, IRModule* module)
{
    IRInst* undefInst = nullptr;

    for (auto inst : module->getModuleInst()->getChildren())
    {
        if (inst->getOp() == kIROp_Poison && inst->getDataType() &&
            inst->getDataType()->getOp() == kIROp_VoidType)
        {
            undefInst = inst;
            break;
        }
    }
    if (!undefInst)
    {
        auto voidType = builder.getVoidType();
        builder.setInsertAfter(voidType);
        undefInst = builder.emitPoison(voidType);
    }
    return undefInst;
}

IROp getSwapSideComparisonOp(IROp op)
{
    switch (op)
    {

Review comment (suggestion):
The name has `Unit` in it, but it actually return `void` type Poison, should we change the name?
Also `getUnitPoisionVal` => `getUnitPoisonVal`

Code after:
        break;
    case kIROp_CastPtrToInt:
    case kIROp_Reinterpret:
    case kIROp_BitCast:
        {
            // If we are trying to cast an address to something else, return true.
            if (isPtrLikeOrHandleType(inst->getOperand(0)->getDataType()) &&
                canAddressesPotentiallyAlias(func, inst->getOperand(0), addr))
                return true;
            else if (!isValueType(inst->getOperand(0)->getDataType()))
            {
                // This is some unknown handle type, we assume it can have any side effects.
                return true;
            }
        }
        break;
    default:
        // Default behavior is that any insts that have side effect may affect `addr`.
        if (inst->mightHaveSideEffects())
            return true;
        break;
    }
    return false;
}

IRInst* getUnitPoisonVal(IRBuilder builder, IRModule* module)
{
    IRInst* undefInst = nullptr;

    for (auto inst : module->getModuleInst()->getChildren())
    {
        if (inst->getOp() == kIROp_Poison && inst->getDataType() &&
            inst->getDataType()->getOp() == kIROp_VoidType)
        {
            undefInst = inst;
            break;
        }
    }
    if (!undefInst)
    {
        auto voidType = builder.getVoidType();
        builder.setInsertAfter(voidType);
        undefInst = builder.emitPoison(voidType);
    }
    return undefInst;
}

IROp getSwapSideComparisonOp(IROp op)
{
    switch (op)
    {
Repository: shader-slang/slang
File: source/slang/slang-ir.cpp (C++)

Code before:
        // uses will become the start
        // of the list of uses for
        // `other`
        other->firstUse = ff;
        ff->prevLink = &other->firstUse;

        // And `this` will have no uses any more.
        thisInst->firstUse = nullptr;

        ff->debugValidate();
    }

    for (auto setInst : setsToUpdate)
    {
        auto module = setInst->getModule();
        IRBuilder builder(module);

        HashSet<IRInst*>& elements = *module->getContainerPool().getHashSet<IRInst>();

        for (UInt i = 0; i < setInst->getOperandCount(); i++)
            elements.add(setInst->getElement(i));

        auto newSetInst = builder.getSet(setInst->getOp(), elements);
        if (newSetInst != setInst)
        {
            // setInst was improperly ordered, so we need to replace its uses.
            // and then delete it.
            //
            setInst->replaceUsesWith(newSetInst);
            setInst->removeAndDeallocate();
        }

        module->getContainerPool().free(&elements);
    }
}

void IRInst::replaceUsesWith(IRInst* other)
{
    _replaceInstUsesWith(this, other);
}

// Insert this instruction into the same basic block
// as `other`, right before it.
void IRInst::insertBefore(IRInst* other)
{
    SLANG_ASSERT(other);
    if (other->getPrevInst() == this)
        return;
    if (other == this)
        return;
    _insertAt(other->getPrevInst(), other, other->getParent());

Review comment (suggestion):
```suggestion
            // setInst was improperly ordered, so we need to replace its uses
```

Code after:
                    // If existingVal has been replaced by something else, use that.
                    dedupContext->getInstReplacementMap().tryGetValue(existingVal, existingVal);
                    addToWorkList(user, existingVal);
                }
                else
                {
                    dedupContext->_addGlobalNumberingEntry(user);
                }
            }

            // Try to move to the next use, but bail
            // out if we are at the last one.
            IRUse* nn = uu->nextUse;
            if (!nn)
                break;

            uu = nn;
        }

        // We are at the last use (and there must
        // be at least one, because we handled
        // the case of an empty list earlier).
        SLANG_ASSERT(uu);

        // Our job at this point is to splice
        // our list of uses onto the other
        // value's uses.
        //
        // If the value already had uses, then
        // we need to patch our new list onto
        // the front.
        if (auto nn = other->firstUse)
        {
            uu->nextUse = nn;
            nn->prevLink = &uu->nextUse;
        }

        // No matter what, our list of
        // uses will become the start
        // of the list of uses for
        // `other`
        other->firstUse = ff;
        ff->prevLink = &other->firstUse;

        // And `this` will have no uses any more.
        thisInst->firstUse = nullptr;

        ff->debugValidate();
    }

    for (auto setInst : setsToUpdate)
Repository: shader-slang/slang
File: source/slang/slang-reflection-json.cpp (C++)

Code before:
{
    if (varLayout->getCategoryCount() != 0)
    {
        for (unsigned int categoryIdx = 0; categoryIdx < varLayout->getCategoryCount();
             categoryIdx++)
        {
            auto category = varLayout->getCategoryByIndex(categoryIdx);
            if (category == slang::MetalArgumentBufferElement)
            {
                return g_inProgramLayout->getTypeLayout(
                    varLayout->getTypeLayout()->getType(),
                    slang::LayoutRules::MetalArgumentBufferTier2);
            }
        }
    }
    return nullptr;
}

static void emitReflectionVarLayoutJSON(PrettyWriter& writer, slang::VariableLayoutReflection* var)
{
    writer << "{\n";
    writer.indent();

    CommaTrackerRAII commaTracker(writer);

    if (var->getVariable())
    {
        if (auto name = var->getName())
        {
            writer.maybeComma();
            emitReflectionNameInfoJSON(writer, name);
        }
    }

    writer.maybeComma();
    writer << "\"type\": ";
    if (auto newTypeLayout = maybeChangeTypeLayoutToAgumentBufferTier2(var))
    {
        emitReflectionTypeLayoutJSON(writer, newTypeLayout);
    }
    else
    {
        emitReflectionTypeLayoutJSON(writer, var->getTypeLayout());
    }

    if (auto variable = var->getVariable())
    {
        emitReflectionModifierInfoJSON(writer, variable);
    }

    emitReflectionVarBindingInfoJSON(writer, var);

Review comment (bug):
I think the fix should be in
```
char const* getName() { return getVariable()->getName(); }
```
I like to suggest to change to:
```
char const* getName()
{
    if (auto var = getVariable())
      return var->getName();
    return nullptr;
}

Code after:
{
    if (varLayout->getCategoryCount() != 0)
    {
        for (unsigned int categoryIdx = 0; categoryIdx < varLayout->getCategoryCount();
             categoryIdx++)
        {
            auto category = varLayout->getCategoryByIndex(categoryIdx);
            if (category == slang::MetalArgumentBufferElement)
            {
                return g_inProgramLayout->getTypeLayout(
                    varLayout->getTypeLayout()->getType(),
                    slang::LayoutRules::MetalArgumentBufferTier2);
            }
        }
    }
    return nullptr;
}

static void emitReflectionVarLayoutJSON(PrettyWriter& writer, slang::VariableLayoutReflection* var)
{
    writer << "{\n";
    writer.indent();

    CommaTrackerRAII commaTracker(writer);

    if (auto name = var->getName())
    {
        writer.maybeComma();
        emitReflectionNameInfoJSON(writer, name);
    }

    writer.maybeComma();
    writer << "\"type\": ";
    if (auto newTypeLayout = maybeChangeTypeLayoutToAgumentBufferTier2(var))
    {
        emitReflectionTypeLayoutJSON(writer, newTypeLayout);
    }
    else
    {
        emitReflectionTypeLayoutJSON(writer, var->getTypeLayout());
    }

    if (auto variable = var->getVariable())
    {
        emitReflectionModifierInfoJSON(writer, variable);
    }

    emitReflectionVarBindingInfoJSON(writer, var);
    writer.dedent();
    writer << "\n}";
}