- moving string parameter into data member instead of copying it. #1899

yves-dolce · 2019-07-10T00:03:58Z

removing noexcept from methods where an exception could be raised.
If std::terminate() call is desired instead, I guess those should be
left and std::move_if_noexcept() used to document the fact that it's
on purpose.
std::move local variable into argument when possible.
change maxversiontested XML element to maxVersionTested.
used of gsl::narrow_cast where appropriate to prevent warnings.
fixed bug in TerminalSettings::SetColorTableEntry()

Summary of the Pull Request

References

PR Checklist

Closes Profile::Set...(std::wstring param) not using std::move #1844
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

- removing noexcept from methods where an exception could be raised. If std::terminate() call is desired instead, I guess those should be left and std::move_if_noexcept() used to document the fact that it's on purpose. - std::moving local variable into argument when possible. - change maxversiontested XML element to maxVersionTested. - used of gsl::narrow_cast where appropriate to prevent warnings. - fixed bug in TerminalSettings::SetColorTableEntry()

msftclas · 2019-07-10T00:04:11Z

All CLA requirements met.

zadjii-msft

I mostly have some questions, otherwise this largely looks fine.

zadjii-msft · 2019-07-16T13:57:09Z

src/cascadia/TerminalApp/CascadiaSettings.cpp

    {
        auto pwshProfile{ _CreateDefaultProfile(L"PowerShell Core") };
-        pwshProfile.SetCommandline(psCoreCmdline);
+        pwshProfile.SetCommandline(std::move(psCoreCmdline));


Out of curiousity, how does line 206 (above) work?

powershellProfile.SetCommandline(L"powershell.exe");

Does it create a new wstring using the string literal to initialize it, then immediately move it to the function call? (not saying this is wrong, I just don't know)

BTW, this effectively means that if the creation of the string raises an exception, it will happen at the call site, in CascadiaSettings::_CreateDefaultProfiles(), not in Profile::SetCommandLine() (which is rightfully noexcept).

zadjii-msft · 2019-07-16T13:58:19Z

src/cascadia/TerminalApp/App.cpp


            // If we don't have that many profiles, then do nothing.
-            if (realIndex >= profiles.size())
+            if (realIndex >= gsl::narrow_cast<decltype(realIndex)>(profiles.size()))


Is there a particular reason to use decltype(realIndex) over manually stating the type of realIndex?

If the type is changed for any reason, one would not need to also change that line.
That said, this was a small mistake as I should have used gsl::narrow instead. It will raise an exception if the cast would change the value in any way. (see gsl_util).
I'll wait for you feedback to make the change.

This seems correct to me. I love decltype.

zadjii-msft · 2019-07-16T13:59:23Z

src/cascadia/TerminalApp/Profile.cpp

 void Profile::SetColorScheme(std::optional<std::wstring> schemeName) noexcept
 {
-    _schemeName = schemeName;
+    static_assert(noexcept(_schemeName = std::move(schemeName)));


Wait what does this do? I don't think I'm familiar with using noexcept() as a function like that.

I've only been using it a couple of times myself. 😉
See noexcept operator.
It just documents the fact that line 450 does not raise any exception. If anything changes in std::optional or std::wstring implementations and an exception could be raised, line 449 would effectively raise an error during build time.

My opinion only 😄 - although 100% correct - code that checks to see if standard C++ code is C++ complaint is overkill and adds unnecessary complexity.

I agree that we shouldn't need to validate that C++ code is C++ compliant!

OK.
FWIW, it's about the implementation of the std::optional move constructor which I believe was not marked as noexcept till C++ 17.
I find that static_assert statement to be easier than starting to read the C++ standard. ;-)

zadjii-msft · 2019-07-16T14:00:30Z

src/cascadia/TerminalApp/Profile.cpp

-void Profile::SetIconPath(std::wstring_view path) noexcept
+void Profile::SetIconPath(std::wstring_view path)
 {
+    static_assert(!noexcept(_icon.emplace(path)));


Related to the above, what's this doing and why does this one have a ! in it?

It's the flip side of the previous comment. Maybe it'd be clearer if I'd had the comment for each static_assert:

static_assert(noexcept(_schemeName = std::move(schemeName)), "Profile::SetColorScheme() is no longer noexcept!") static_assert(!noexcept(_icon.emplace(path)), "Mark Profile::SetIconPath() as noexcept");

Yea a comment might be helpful, since I definitely read that code and assumed that it was asserting the opposite :P

DHowett-MSFT · 2019-07-22T22:46:56Z

src/cascadia/TerminalApp/App.cpp


            // If we don't have that many profiles, then do nothing.
-            if (realIndex >= profiles.size())
+            if (realIndex >= gsl::narrow_cast<decltype(realIndex)>(profiles.size()))


This seems correct to me. I love decltype.

src/cascadia/TerminalApp/App.cpp

DHowett-MSFT · 2019-07-22T22:48:34Z

src/cascadia/TerminalApp/Profile.cpp

 void Profile::SetColorScheme(std::optional<std::wstring> schemeName) noexcept
 {
-    _schemeName = schemeName;
+    static_assert(noexcept(_schemeName = std::move(schemeName)));


I agree that we shouldn't need to validate that C++ code is C++ compliant!

DHowett-MSFT · 2019-07-22T22:49:18Z

src/cascadia/TerminalApp/Profile.cpp

 // Arguments:
 // - path: the path
-void Profile::SetIconPath(std::wstring_view path) noexcept
+void Profile::SetIconPath(std::wstring_view path)


Curious here. This one uses wstring_view, and the others use wstring. Should we standardize on just one? If so, what do you think we should do, @yves-dolce?

If the body of the method (e.g. Profile::SetIconPath) calls methods that support std::wstring_view, std::wstring_view will allow you not to have any heap allocation during the parameter passing. That class is just a pointer and a size after all.
But keep in mind that:

it does not enforce the string to be null terminated. Again, it's just a pointer to the beginning of the string and a length.

the compiler might have a harder time optimizing the calling site as now, you've created an alias. When you were taking a std;:wstring, no aliasing was created.

All in all, I'd personally stick with std::wstring, and std::move. I use std::wstring_view if I plan on having to interact with code that uses weird stuff and don't want to force heap allocation... (CString, bstr_t, CComBSTR, ...)

Fair enough! 👍

yves-dolce · 2019-07-23T21:17:12Z

This is the first time I participate on an GitHub open source project.
Can you confirm that I shall merge the latest from master, fix the code base on your comments and push my fork and then this PR will automatically updated.
Is that correct?

DHowett-MSFT · 2019-07-23T21:21:48Z

That's correct!

yves-dolce · 2019-07-23T21:41:17Z

I pushed without compiling as my changes were very small.
I just tried anyway but I now get Mismatched C++/WinRT headers. static assertion failure: I have 2.0.190722.3.
What's the best way to fix this if I want to keep the latest version accessible for my own code?

DHowett-MSFT · 2019-07-23T22:31:05Z

I believe you just need to Clean your local build and make sure you "restore nuget packages" again; our dependencies have been updated 😄

DHowett-MSFT · 2019-07-25T04:48:19Z

src/cascadia/TerminalApp/Profile.cpp

 // Arguments:
 // - path: the path
-void Profile::SetIconPath(std::wstring_view path) noexcept
+void Profile::SetIconPath(std::wstring_view path)


Fair enough! 👍

DHowett-MSFT · 2019-07-25T04:48:57Z

src/cascadia/WindowsTerminal/WindowsTerminal.manifest

        <!-- Windows 10 1903 -->
        <!-- See https://docs.microsoft.com/en-us/windows/apps/desktop/modernize/xaml-islands -->
-        <maxversiontested Id="10.0.18362.0"/>
+        <maxVersionTested Id="10.0.18362.0"/>


I'm moderately worried about whether this is case sensitive. I'll check with the manifest team.

Great. I'm curious about that one as XML is case sensitive.

Wellp, it looks like it is case-sensitive. Somebody owes me a coke, I just haven't figured out who.

yves-dolce · 2019-08-04T17:51:23Z

Are we just waiting for a 2nd reviewer?

zadjii-msft

I'm okay with this, but since @DHowett-MSFT left some other comments, I'll let him be the second on this one.

zadjii-msft · 2019-08-05T13:48:54Z

src/cascadia/TerminalApp/Profile.cpp

 void Profile::SetColorScheme(std::optional<std::wstring> schemeName) noexcept
 {
-    _schemeName = schemeName;
+    _schemeName = std::move(schemeName);


Out of curiosity, this works with a std::optional too?

Yes. Blurb from cppreference.

Move constructor: If other contains a value, initializes the contained value as if direct-initializing (but not direct-list-initializing) an object of type T with the expression std::move(*other) and does not make other empty: a moved-from optional still contains a
value, but the value itself is moved from. If other does not contain a value, constructs an object that does not contain a value. This constructor does not participate in overload resolution unless std::is_move_constructible_v is true. It is a trivial constructor if std::is_trivially_move_constructible_v is true.

So in this case std::move() will cast the variable (schemeName) to an rvalue, allowing it to bind to a move constructor.

miniksa · 2019-08-06T16:57:14Z

@DHowett-MSFT, this one is also waiting for you and otherwise looks good to go.

DHowett-MSFT

I'm quite comfortable with this now. Thank you for your contribution, and sorry it took us so long!

yves-dolce mentioned this pull request Jul 10, 2019

Profile::Set...(std::wstring param) not using std::move #1844

Closed

zadjii-msft reviewed Jul 16, 2019

View reviewed changes

DHowett-MSFT reviewed Jul 22, 2019

View reviewed changes

Yves Dolce added 2 commits July 23, 2019 14:31

PR 1899 comments

68e1230

Merge branch 'master' of https://github.com/microsoft/terminal

12f71fb

carlos-zamora approved these changes Jul 25, 2019

View reviewed changes

DHowett-MSFT reviewed Jul 25, 2019

View reviewed changes

zadjii-msft reviewed Aug 5, 2019

View reviewed changes

miniksa approved these changes Aug 6, 2019

View reviewed changes

miniksa added the Needs-Attention The core contributors need to come back around and look at this ASAP. label Aug 6, 2019

DHowett-MSFT approved these changes Aug 6, 2019

View reviewed changes

DHowett-MSFT merged commit dfb8536 into microsoft:master Aug 6, 2019

- moving string parameter into data member instead of copying it. #1899

- moving string parameter into data member instead of copying it. #1899

Uh oh!

Conversation

yves-dolce commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

msftclas commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yves-dolce Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yves-dolce Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yves-dolce commented Jul 23, 2019

Uh oh!

DHowett-MSFT commented Jul 23, 2019

Uh oh!

yves-dolce commented Jul 23, 2019

Uh oh!

DHowett-MSFT commented Jul 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yves-dolce Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yves-dolce commented Aug 4, 2019

yves-dolce commented Jul 10, 2019 •

edited

Loading

msftclas commented Jul 10, 2019 •

edited

Loading

yves-dolce Jul 16, 2019 •

edited

Loading

yves-dolce Jul 23, 2019 •

edited

Loading

yves-dolce Jul 26, 2019 •

edited

Loading