-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement the FTCS_PROMPT sequence for marking the start of the prompt #13163
Conversation
…erimental-marks # Conflicts: # src/terminal/adapter/ITermDispatch.hpp # src/terminal/adapter/adaptDispatch.hpp # src/terminal/adapter/termDispatch.hpp
This comment was marked as resolved.
This comment was marked as resolved.
return false; | ||
} | ||
|
||
const auto action{ parts[0] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this code requires a string copy. This avoids one:
const auto action{ parts[0] }; | |
const auto& action = parts[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you prefer he not use the uniform initialization syntax (since you switched it to =
) 😉
const auto action{ parts[0] }; | |
const auto& action{ parts[0] }; |
I had to read the rules about const auto&
locals and how they extend lifetime -- I still think it's a bad pattern to encourage, but we use it all over the place \_(shrug)_/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you prefer he not use the uniform initialization syntax (since you switched it to
=
) 😉
The rest of the file uses classic =
assignments. I like consistency. 👉👈
Of course you probably already figured out my deepest secret that - entirely unrelated to this - I personally kinda dislike the uniform initialization syntax. 😅 This is what I write:
auto bar = bar_producer(); // good
auto bar{ bar_producer() }; // "bad"
Foo foo{ bar }; // good
Isn't the uniform initialization syntax nothing but a bolted on language hack, because people were afraid of breaking existing code by making round brackets better? I use it because it's very useful, but would you write this if it was an equivalent thing?
auto bar(bar_producer());
I wouldn't and so I don't use curly brackets either, because I consider curly brackets stricter round brackets. In my mind I'm not constructing an object there, I'm assigning an unnamed rvalue a name via variable assignment. And so I write auto result = foo()
.
I had to read the rules about
const auto&
locals and how they extend lifetime -- I still think it's a bad pattern to encourage, but we use it all over the place \_(shrug)_/
I think about it differently: Here we "bind to the memory address" of the first item in an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultimately, Audit will complain:
error C26445: Do not assign gsl::span or std::string_view to a reference. They are cheap to construct and are not owners of the underlying data.
so, const auto action = ...
it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error C26445: Do not assign gsl::span or std::string_view to a reference.
But passing it as a by-reference argument is fine? smh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the underlying point is: this is not a string copy, it's a string_view copy ;)
…/11000-FTCS-simple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits
My nits are just comment changes? Huh. |
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
Quick note since this is currently the Windows Terminal bug-tracker example for adding There also seems to be an extraneous Edit: I did eventually get it working ( |
Implements the FTCS_PROMPT sequence,
OSC 133 ; A ST
. In this PR, it's just used to set a simple Prompt mark on the current line, in the same way that the iTerm2 sequence works.There's rumination in #11000 on how to implement the rest of the FTCS sequences.
This is broken into its own PR at the moment. Quoth j4james:
This part of the plumbing is super easy, so I thought it would be valuable to add regardless if we get to the whole of FTCS in 1.15.
$PROFILE
: