[StubGen] Fix optional vectors

[sebaszm]

additional test cases:
Set optional vector round trip.
Unset optional vector round trip.
Set optional vector round trip to the same vector parameter.
Unset optional vector round trip to the same vector parameter.
Set optional vector change into unset in the same vector parameter.
Set optional vector in a struct round trip.
Unset optional vector in a struct round trip.
Set optional vector in a struct round trip to the same vector parameter.
Unset optional vector in a struct round trip to the same vector sparameter.
Set optional vector in a struct change to unset in the same vector parameter.

[github-actions]

LuaGenerator Results

View Results

No changes detected.

[Copilot]

Pull request overview

This PR extends the FunctionalTests “optionals” surface area to include optional std::vector<uint8_t> (including vectors inside an optional struct), and updates the ProxyStubGenerator so optional vectors deserialize correctly.

Changes:

• Add new COM-RPC functional tests covering optional vectors (in/out, inout, and nested-in-struct cases).
• Extend ITestOptionals with a Compound struct and new optional-vector RPC methods, plus server-side implementations.
• Adjust ProxyStubGenerator/StubGenerator.py to correctly build dynamic arrays for optional parameters (using a temporary object and proper assignment/unset handling).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
tests/FunctionalTests/comrpc/tests/TestOptionals.cpp	Adds functional tests validating optional vector round-trips and struct-embedded optional vectors.
tests/FunctionalTests/common/interfaces/ITestOptionals.h	Adds Compound and new RPC method declarations for optional vectors.
tests/FunctionalTests/common/implementations/TestOptionalsImpl.cpp	Implements the new optional-vector methods used by the functional tests.
ProxyStubGenerator/StubGenerator.py	Fixes stub generation for optional dynamic arrays (vectors), including correct temp storage and unset behavior.

Suppressed comments (1)

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:272

• Similar to the plain optional-vector test: optOutput.Value() is used later in this test after only non-fatal EXPECT_EQ(optOutput.IsSet(), true). If the optional isn’t set, Value() may assert/crash. Prefer ASSERT_TRUE(optOutput.IsSet()) before accessing Value(), and use ASSERT_* for prerequisites (e.g., sizes) before iterating/indexing.

    ASSERT_EQ(_proxy->ProcessOptionalVectorInOptionalStruct(optInput, optOutput), Core::ERROR_NONE);
    EXPECT_EQ(optOutput.IsSet(), true);
    EXPECT_EQ(optOutput.Value().magic, data.magic);
    EXPECT_EQ(optOutput.Value().optionalMagic.IsSet(), true);
    EXPECT_EQ(optOutput.Value().optionalMagic.Value(), data.optionalMagic.Value());

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:234

• optData.Value() is used after EXPECT_EQ(optData.IsSet(), true). If IsSet() is false, Value() may assert/UB and abort the test. Use ASSERT_TRUE(optData.IsSet()) (or ASSERT_EQ(..., true)) before dereferencing the optional.

    ASSERT_EQ(_proxy->ProcessOptionalInlineVector(optData, false), Core::ERROR_NONE);
    EXPECT_EQ(optData.IsSet(), true);
    EXPECT_EQ(optData.Value().size(), data.size());
    if (optData.Value().size() == data.size()) {

tests/FunctionalTests/common/implementations/TestOptionalsImpl.cpp:196

• Use of the alternative token and is inconsistent with the rest of this file (which uses &&) and can be problematic under some toolchains/style settings. Prefer && here for consistency.

                if ((data.Value().optionalMagic.IsSet() == true) and (unset == true)) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:210

• optOutput.Value() is accessed immediately after an EXPECT_EQ(optOutput.IsSet(), true). If the optional is unexpectedly unset, Value() may assert/UB and crash the test instead of reporting a clean failure. Prefer ASSERT_TRUE(optOutput.IsSet()) (or ASSERT_EQ(..., true)) before any Value() access in this test.

    ASSERT_EQ(_proxy->ProcessOptionalVector(optInput, optOutput), Core::ERROR_NONE);
    EXPECT_EQ(optOutput.IsSet(), true);
    EXPECT_EQ(optOutput.Value().size(), input.size());
    if (optOutput.Value().size() == input.size()) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:344

• Typo in comment: "sparameter" should be "parameter".

    // Unset optional vector in a struct round trip to the same vector sparameter.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (11)

Previously missed (2) — in code that hasn't changed since the last review.

tests/FunctionalTests/common/implementations/TestOptionalsImpl.cpp:190

• In this class, methods are consistently declared as "Core::hresult ... override" without a leading "virtual". The extra "virtual" here is redundant (override already implies virtual) and inconsistent with the rest of the file; consider removing it for consistency.

        virtual Core::hresult ProcessOptionalVectorInOptionalInlineStruct(
            Core::OptionalType<Compound>& data,
            const bool unset) override
        {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:212

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

This issue also appears in the following locations of the same file:

• line 236
• line 276
• line 284
• line 305
• line 329
• ...and 3 more

        for (uint8_t i = 0; i < input.size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:344

• Typo in comment: "sparameter" → "parameter".

    // Unset optional vector in a struct round trip to the same vector sparameter.

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:236

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:276

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.data.size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:284

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.optionalData.Value().size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:305

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.data.size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:329

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.data.size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:337

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.optionalData.Value().size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:357

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.data.size(); i++) {

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:380

• Using uint8_t as the loop index can overflow/wrap for vectors larger than 255 elements, potentially causing an infinite loop. Use size_t (or a range-based for loop) for indexing/comparing against std::vector::size().

        for (uint8_t i = 0; i < data.data.size(); i++) {

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:215

• The ASSERT_EQ(...size...) immediately followed by if (...size() == ...) is redundant: if the ASSERT fails the test aborts, so the if never protects anything. Remove the if block and keep the loop after the ASSERT (or change the size check to EXPECT_EQ if you want the loop to be conditionally skipped).

    ASSERT_EQ(optOutput.Value().size(), input.size());
    if (optOutput.Value().size() == input.size()) {
        // process multiplies by 2
        for (uint8_t i = 0; i < input.size(); i++) {
            EXPECT_EQ(optOutput.Value()[i], input[i]*2);
        }
    }

tests/FunctionalTests/common/interfaces/ITestOptionals.h:153

• Docstring has a grammatical issue and double space: // @brief Process vector in a struct. Consider changing to // @brief Process a vector in a struct. for clarity and consistency with the other brief comments.

        // @brief Process  vector in a struct
        virtual Core::hresult ProcessOptionalVectorInOptionalStruct(
            const Core::OptionalType<Compound>& input,
            Core::OptionalType<Compound>& output /* @out */) = 0;

tests/FunctionalTests/comrpc/tests/TestOptionals.cpp:209

• These new tests use ASSERT_EQ(optX.IsSet(), true/false); elsewhere in this file the convention is ASSERT_TRUE/EXPECT_FALSE for boolean checks (e.g. around line 73). Switching to the boolean-specific assertions will produce clearer failure messages and match the existing style.

    ASSERT_EQ(_proxy->ProcessOptionalVector(optInput, optOutput), Core::ERROR_NONE);
    ASSERT_EQ(optOutput.IsSet(), true);
    ASSERT_EQ(optOutput.Value().size(), input.size());