Refactored declaring multiple variables on a single line - #550
Conversation
This reverts commit 7e5b7b3.
Co-Authored-By: Casey Carter <cartec69@gmail.com>
|
Are the |
|
Oop nope:( ! I think It changed when I pulled from STL/master to mine |
Stephan T. Lavavej (StephanTLavavej)
left a comment
There was a problem hiding this comment.
Looks good, thank you! I observe that the /* const */ comments in ymath.h are being dropped, but that's fine (those variables should be const, but we can't do that because of binary compatibility; in vNext the entire area needs to be completely overhauled so we don't need additional reminders).
I'll go ahead and port this to the Microsoft-internal repo so we can merge it.
One comment about your PRs - if you look at the PR history, it shows a long sequence of commits (17 in this case), with your previous fixes and merges from master. Working in a long-lived branch is fine, but when you submit a new PR, I recommend creating a fresh branch, based on top of the current origin/master, containing one or more commits that are purely for that specific PR. (Usually one squashed commit is best for your initial PR, but it can be structured as a sequence of commits if your changes are unusually complicated; that is, commits in a PR should be arranged for clarity to reviewers, and not as a literal record of history.)
git experts can probably explain better techniques for squashing commits, but what I do is git rebase -i master when that works. When it doesn't (e.g. due to complicated merge history), what I do is:
S:\GitHub\STL>git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
S:\GitHub\STL>git pull
Already up to date.
S:\GitHub\STL>git checkout fix-avoid-declaring-multiple-var
Switched to a new branch 'fix-avoid-declaring-multiple-var'
Branch 'fix-avoid-declaring-multiple-var' set up to track remote branch 'fix-avoid-declaring-multiple-var' from 'jeanphilippekernel'.
S:\GitHub\STL>git merge master
Merge made by the 'recursive' strategy.
llvm-project | 2 +-
stl/inc/future | 11 +--
[... more output ...]
35 files changed, 341 insertions(+), 214 deletions(-)
S:\GitHub\STL>git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
S:\GitHub\STL>git checkout -b GH-550
Switched to a new branch 'GH-550'
S:\GitHub\STL>git diff master fix-avoid-declaring-multiple-var > C:\Temp\GH-550.patch
S:\GitHub\STL>git apply C:\Temp\GH-550.patch
S:\GitHub\STL>git add --all
S:\GitHub\STL>git commit -m "GH-550 Refactored declaring multiple variables on a single line"
[GH-550 bf5a8a4] GH-550 Refactored declaring multiple variables on a single line
11 files changed, 86 insertions(+), 38 deletions(-)
This is part of my process for porting changes from GitHub to Microsoft-internal git, but it can also be used to create PRs with simplified history.
|
Oh great !! Thank you so much for the technique !!! |
|
If you aren't transferring between repos the diff/apply step can be reduced to reset, assuming you are already on your branch that you're trying to submit: |
|
Jean Philippe (@JeanPhilippeKernel) - thanks for improving the codebase yet again. :-) Billy O'Neal (@BillyONeal) - oh, that makes even more sense, thanks for teaching us. |
|
It's my pleasure and I feel so honored to work with y'all :) |
Description
Fixes #522.
Checklist
Be sure you've read README.md and understand the scope of this repo.
If you're unsure about a box, leave it unchecked. A maintainer will help you.
_Uglyas perhttps://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.