microsoft / microsoft/wil

What's the expected result of wil::make_unique_string<unique_hlocal_string>(L"hello", 100); ?

Open
#142 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

discussion
Dominant language
C++
Stars
3k
Forks
300
Avg merge
19h 12m
Merged PRs (30d)
1

Description

auto str = wil::make_unique_string<unique_hlocal_string>(L"hello", 100);
auto equals = wcscmp(str.get(), L"hello");

Q: How big is the memory allocation supposed to be?

  1. 10 bytes
  2. 12 bytes
  3. 200 bytes
  4. 202 bytes

Unless I'm misreading something the answer is #4
length = 100. AllocatedBytes = (length + 1) * sizeof(wchar_t) == 202 bytes to hold 101 wide characters
str[0] = L'h'
str[1] = L'e'
str[2] = L'l'
str[3] = L'l'
str[4] = L'o'
str[5] = L'\0'
str[100] = L'\0'
str[6...99] are unassigned

Is this intentional?

As implemented specifying a length longer than the supplied string will allocated a larger buffer than necessary but the doccomment for resource.h line 3220+ and comments in the function don't mention this.

The same with a null parameter produces a similar result

auto str = wil::make_unique_string<unique_hlocal_string>(nullptr, 100);

length = 0, AllocatedBytes=202
str[0] = L'\0'
str[100] = L'\0'
str[1...99] are unassigned
No such mention in the doccomment but this is explicitly stated in the comment @ line 3246

        // When the source string exists, calculate the number of characters to copy up to either
        // 1) the length that is given
        // 2) the length of the source string. When the source does not exist, use the given length
        //    for calculating both the size of allocated buffer and the number of characters to copy.

The results match #2

  • "When the source does not exist" - source=nullptr so check
  • "use the given length for calculating both the size of allocated buffer" - allocation=bytes for length+1 characters so check
  • "and the number of characters to copy" - huh? "When the source does not exist, use the given length for calculating...the number of characters to copy". If we've got no source use length to compute how much source to copy?
    Shouldn't this read
        // 2) the length of the source string. When the source does not exist, use the given length
        //    for calculating the size of allocated buffer.

If length > wcslen(source) is supposed to allocate a larger buffer than strictly necessary for source the doccomment needs update. Also, that embedded comment "...number of characters to copy up to either" should be reworded to more clearly state the LARGER of the two will be used.

If the intent is to use min(length, wcslen(source)) then the code and that embedded comment needs update.

I found it a bit surprising behaviorally, given no mention in doccomments or hint in the interface e.g. there's no wil::make_unique_string(length) overload to just allocate a buffer of specified size.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read resource.h around the doccomment at line 3220 and the implementation comment near line 3246, then trace wil::make_unique_string with a longer source length and with nullptr. Resolve whether the current allocation behavior is intentional; done means the code and documentation consistently describe the chosen behavior, with any needed coverage added where the project’s existing tests for this utility live.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
tooling
Issue type
Documentation
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.