Skip to content

Use iterators in FromJson to avoid copies - #41641

Open
Blue (OneBlue) wants to merge 4 commits into
masterfrom
user/oneblue/json
Open

Blue (OneBlue) wants to merge 4 commits into
masterfrom
user/oneblue/json

Conversation

@OneBlue

Copy link
Copy Markdown
Collaborator

Summary of the Pull Request

This change reworks the json parsing logic to work with iterators, which avoids having to copy the entire json into a std::string in some places

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Copilot AI lite review requested due to automatic review settings September 17, 2026 23:44
@OneBlue
Blue (OneBlue) requested review from a team as code owners September 17, 2026 23:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new FromJson template has compile-breaking template-parameter defaults and fragile iterator-to-std::string_view construction that should be corrected before merging.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR refactors shared JSON deserialization helpers to support parsing directly from iterator ranges, enabling callers to avoid allocating/copying buffers into temporary std::strings before parsing. It also updates WSLc image-import progress parsing to use the new iterator-based API.

Changes:

  • Added an iterator-range FromJson overload and routed existing const char* / const wchar_t* overloads through it.
  • Updated WSLCSession::ImportImageImpl progress parsing to parse JSON directly from the received buffer iterators.
File summaries
File Description
src/windows/wslcsession/WSLCSession.cpp Uses iterator-based FromJson for import-progress parsing to avoid an intermediate std::string copy.
src/shared/inc/JsonUtils.h Introduces iterator-range JSON parsing and updates overloads/error reporting accordingly.
Review details

Suppressed comments (2)

src/shared/inc/JsonUtils.h:82

  • const std::string_view value{First, Last}; depends on the C++23 iterator-range constructor and on Iterator being contiguous; this is likely to break builds or callers that pass non-contiguous iterators. Since this is only used on the exception path, consider materializing a std::string from the iterator range for logging/error text instead.
        const std::string_view value{First, Last};

#ifdef WIN32

        THROW_HR_WITH_USER_ERROR_MSG(

src/shared/inc/JsonUtils.h:101

  • FromJson(const char* Value) uses strlen(Value) but this header doesn’t include <cstring> (and on Windows stringshared.h doesn’t include it either), so this can be a fragile/implicit dependency. Prefer std::char_traits<char>::length(Value) (available via <string>) to keep the header self-contained.
T FromJson(const char* Value)
{
    return FromJson<T, TJson>(Value, Value + strlen(Value));
}
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment thread src/shared/inc/JsonUtils.h Outdated
Copilot AI review requested due to automatic review settings September 21, 2026 17:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

FromJson(const char*) introduces a new strlen() dependency in a header without ensuring the required include, creating a fragile transitive-include build risk.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add explicit <cstring> include for strlen dependency

src/​shared/​inc/​JsonUtils.h:97

FromJson(const char* Value) now depends on strlen() but this header doesn’t include <cstring>, so it relies on transitive includes and can break depending on include order/toolset. Prefer a length computation that doesn’t introduce a new header dependency (or explicitly include <cstring>).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants