Repository: shader-slang/slang
File: source/compiler-core/slang-nvrtc-compiler.cpp (C++)

Code before:
    // Add defines
    for (const auto& define : options.defines)
    {
        StringBuilder builder;
        builder << "-D";
        builder << asStringSlice(define.nameWithSig);
        if (define.value.count)
        {
            builder << "=" << asStringSlice(define.value);
        }

        cmdLine.addArg(builder);
    }

    // Add includes
    for (const auto& include : options.includePaths)
    {
        cmdLine.addArg("-I");
        cmdLine.addArg(asString(include));
    }

    if (SLANG_FAILED(_maybeAddHalfSupport(options, cmdLine, diagnostics)))
    {
        diagnostics->setResult(SLANG_FAIL);
        *outArtifact = artifact.detach();
        return SLANG_OK;
    }

    // Neither of these options are strictly required, for general use of nvrtc,
    // but are enabled to make use withing Slang work more smoothly
    {
        // Require c++17, the default at the time of writing, since we share
        // some functionality between slang itself and the compiled code
        cmdLine.addArg("-std=c++17");

        // Disable all warnings
        // This is arguably too much - but nvrtc does not appear to have a mechanism
        // to switch off individual warnings. I tried the -Xcudafe mechanism but
        // that does not appear to work for nvrtc
        cmdLine.addArg("-w");
    }

    {
        // The lowest supported CUDA architecture version supported
        // by any version of NVRTC we support is `compute_30`.
        //
        SemanticVersion version(3);

        // Newer releases of NVRTC only support newer CUDA architectures.
        if (m_desc.version.m_major > 12 ||
            (m_desc.version.m_major == 12 && m_desc.version.m_minor >= 8))

Review comment (bug):
Before this PR, it was returning the failing return value from `_maybeAddHalfSupport()`.
Shouldn't we keep the same behavior rather than returning SLANG_OK?

Code after:
    // Add defines
    for (const auto& define : options.defines)
    {
        StringBuilder builder;
        builder << "-D";
        builder << asStringSlice(define.nameWithSig);
        if (define.value.count)
        {
            builder << "=" << asStringSlice(define.value);
        }

        cmdLine.addArg(builder);
    }

    // Add includes
    for (const auto& include : options.includePaths)
    {
        cmdLine.addArg("-I");
        cmdLine.addArg(asString(include));
    }

    if (SLANG_FAILED(_maybeAddHalfSupport(options, cmdLine, diagnostics)))
    {
        diagnostics->setResult(SLANG_FAIL);
        *outArtifact = artifact.detach();
        return SLANG_FAIL;
    }

    // Neither of these options are strictly required, for general use of nvrtc,
    // but are enabled to make use withing Slang work more smoothly
    {
        // Require c++17, the default at the time of writing, since we share
        // some functionality between slang itself and the compiled code
        cmdLine.addArg("-std=c++17");

        // Disable all warnings
        // This is arguably too much - but nvrtc does not appear to have a mechanism
        // to switch off individual warnings. I tried the -Xcudafe mechanism but
        // that does not appear to work for nvrtc
        cmdLine.addArg("-w");
    }

    {
        // The lowest supported CUDA architecture version supported
        // by any version of NVRTC we support is `compute_30`.
        //
        SemanticVersion version(3);

        // Newer releases of NVRTC only support newer CUDA architectures.
        if (m_desc.version.m_major > 12 ||
            (m_desc.version.m_major == 12 && m_desc.version.m_minor >= 8))