Repository: bitcoin/bitcoin
File: src/script/script.h (C/C++)

Code before:
protected:
    CScript& push_int64(int64_t n)
    {
        if (n == -1 || (n >= 1 && n <= 16))
        {
            push_back(n + (OP_1 - 1));
        }
        else if (n == 0)
        {
            push_back(OP_0);
        }
        else
        {
            *this << CScriptNum::serialize(n);
        }
        return *this;
    }

public:
    CScript() = default;
    template <std::input_iterator InputIterator>
    CScript(InputIterator first, InputIterator last) : CScriptBase{first, last} { }

    SERIALIZE_METHODS(CScript, obj) { READWRITE(AsBase<CScriptBase>(obj)); }

    explicit CScript(int64_t b) { operator<<(b); }
    explicit CScript(opcodetype b)     { operator<<(b); }
    explicit CScript(const CScriptNum& b) { operator<<(b); }
    // delete non-existent constructor to defend against future introduction
    // e.g. via prevector
    explicit CScript(const std::vector<unsigned char>& b) = delete;

    /** Delete non-existent operator to defend against future introduction */
    CScript& operator<<(const CScript& b) = delete;

    CScript& operator<<(int64_t b) LIFETIMEBOUND { return push_int64(b); }

    CScript& operator<<(opcodetype opcode) LIFETIMEBOUND
    {
        if (opcode < 0 || opcode > 0xff)
            throw std::runtime_error("CScript::operator<<(): invalid opcode");
        insert(end(), (unsigned char)opcode);
        return *this;
    }

    CScript& operator<<(const CScriptNum& b) LIFETIMEBOUND
    {
        *this << b.getvch();
        return *this;
    }


Review comment (performance):
not sure about the naming here. This `PushData` does not push any data (at least not in a sense what the `OP_PUSHDATA*` are referring to). `InsertData` may be a better name. However, this would also be confusing, because the method is private and just a wrapper around the public `insert` method. It seems easier to just call `insert` directly instead of creating another wrapper.

Also, it may be easier to review to leave the code where it was, instead of moving it and coming up with new function names. The "deprecated" wrapper could be a simple `return *this << std::as_bytes(b);` which compilers should be able to optimize.

Code after:
{
private:
    inline void AppendDataSize(const uint32_t size)
    {
        if (size < OP_PUSHDATA1) {
            insert(end(), static_cast<value_type>(size));
        } else if (size <= 0xff) {
            insert(end(), OP_PUSHDATA1);
            insert(end(), static_cast<value_type>(size));
        } else if (size <= 0xffff) {
            insert(end(), OP_PUSHDATA2);
            value_type data[2];
            WriteLE16(data, size);
            insert(end(), std::cbegin(data), std::cend(data));
        } else {
            insert(end(), OP_PUSHDATA4);
            value_type data[4];
            WriteLE32(data, size);
            insert(end(), std::cbegin(data), std::cend(data));
        }
    }

    void AppendData(std::span<const value_type> data)
    {
        insert(end(), data.begin(), data.end());
    }

protected:
    CScript& push_int64(int64_t n)
    {
        if (n == -1 || (n >= 1 && n <= 16))
        {
            push_back(n + (OP_1 - 1));
        }
        else if (n == 0)
        {
            push_back(OP_0);
        }
        else
        {
            *this << CScriptNum::serialize(n);
        }
        return *this;
    }

public:
    CScript() = default;
    template <std::input_iterator InputIterator>
    CScript(InputIterator first, InputIterator last) : CScriptBase{first, last} { }

    SERIALIZE_METHODS(CScript, obj) { READWRITE(AsBase<CScriptBase>(obj)); }