Skip to content
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

Add X Macro for fun and for profit #9667

Merged
3 commits merged into from
Mar 31, 2021
Merged

Add X Macro for fun and for profit #9667

3 commits merged into from
Mar 31, 2021

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

This PR adds an X Macro for defining our ShortcutActions. This means that you can add the action in one place, and have the macro synthesize all sorts of boilerplate for you!

From the AllShortcutActions.h file:

For a clearer explanation of how this file should be used, see:
https://en.wikipedia.org/wiki/X_Macro

Include this file to be able to quickly define some code in the exact same
way for every single shortcut action. To use:

  1. Include this file

  2. Define the ON_ALL_ACTIONS macro with what you want each action to show up
    as. Ex:

    #define ON_ALL_ACTIONS(action) void action##Handler();

  3. Then, use the ALL_SHORTCUT_ACTIONS macro to get the ON_ALL_ACTIONS marcro
    repeated once for every ShortcutAction

This is used in KeyMapping.idl, ShortcutAction., TerminalPage., etc. to
reduce the number of places where we must copy-paste boiler-plate code for
each action. This is NOT something that should be used when any individual
case should be customized.

PR Checklist

  • Scratches an itch
  • I work here
  • Tests passed
  • [n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

Originally I had this blocked as a follow up to #9662. However, I've grown tired after a month of merging main into this branch, and I'm just shipping it separately. It will inevitably conflict with anyone who has actions in flight currently.

Validation Steps Performed
The code still builds exactly as before!

**Summary of the Pull Request**

This PR adds an X Macro for defining our ShortcutActions. This means that you can add the action in one place, and have the macro synthesize all sorts of boilerplate for you!

From the `AllShortcutActions.h` file:

> For a clearer explanation of how this file should be used, see:
> https://en.wikipedia.org/wiki/X_Macro
>
> Include this file to be able to quickly define some code in the exact same
> way for _every single shortcut action_. To use:
>
> 1. Include this file
> 2. Define the ON_ALL_ACTIONS macro with what you want each action to show up
>    as. Ex:
>
>    #define ON_ALL_ACTIONS(action) void action##Handler();
>
> 3. Then, use the ALL_SHORTCUT_ACTIONS macro to get the ON_ALL_ACTIONS marcro
>    repeated once for every ShortcutAction
>
> This is used in KeyMapping.idl, ShortcutAction.*, TerminalPage.*, etc. to
> reduce the number of places where we must copy-paste boiler-plate code for
> each action. This is _NOT_ something that should be used when any individual
> case should be customized.

**PR Checklist**
* [x] Scratches an itch
* [x] I work here
* [x] Tests passed
* [n/a] Requires documentation to be updated

**Detailed Description of the Pull Request / Additional comments**

Originally I had this blocked as a follow up to #9662. However, I've grown tired after a month of merging main into this branch, and I'm just shipping it separately. It will inevitably conflict with anyone who has actions in flight currently.

**Validation Steps Performed**
The code still builds exactly as before!
@github-actions
Copy link

Misspellings found, please review:

  • marcro
To accept these changes, run the following commands from this repository on this branch
pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aef aspnet boostorg BSODs Cac COINIT dahall DEFAPP DEFCON fde fea fmtlib HPCON isocpp mintty NVDA pinam QOL remoting Unk unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/9d729a50b03c2e1cc9d6de1fb55c88a314c1d107.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"cac coinit defapp hpcon marcro Remoting unk "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@DHowett
Copy link
Member

DHowett commented Mar 30, 2021

100%. 1 0 0 %

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 30, 2021
Copy link
Collaborator

@skyline75489 skyline75489 left a comment

Choose a reason for hiding this comment

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

Programming 101: less repeating, the better.

@zadjii-msft
Copy link
Member Author

zadjii-msft commented Mar 31, 2021

@miniksa @DHowett when did this get busted in main?

 67>C:\a\1\s\src\host\exe\exemain.cpp(70,22): error C2220: the following warning is treated as an error [C:\a\1\s\src\host\exe\Host.EXE.vcxproj]
    63>ClCompile:
         CopyToCharPopupTests.cpp
##[warning]src\host\exe\exemain.cpp(70,22): Warning C4456: declaration of 'dwValue' hides previous local declaration
         DbcsTests.cpp
    67>C:\a\1\s\src\host\exe\exemain.cpp(70,22): warning C4456: declaration of 'dwValue' hides previous local declaration [C:\a\1\s\src\host\exe\Host.EXE.vcxproj]
       C:\a\1\s\src\host\exe\exemain.cpp(60,11): message : see declaration of 'dwValue' [C:\a\1\s\src\host\exe\Host.EXE.vcxproj]

EDIT: looks like it was fixed with a merge from inbox.

@miniksa
Copy link
Member

miniksa commented Mar 31, 2021

@miniksa @DHowett when did this get busted in main?

 67>C:\a\1\s\src\host\exe\exemain.cpp(70,22): error C2220: the following warning is treated as an error [C:\a\1\s\src\host\exe\Host.EXE.vcxproj]
    63>ClCompile:
         CopyToCharPopupTests.cpp
##[warning]src\host\exe\exemain.cpp(70,22): Warning C4456: declaration of 'dwValue' hides previous local declaration
         DbcsTests.cpp
    67>C:\a\1\s\src\host\exe\exemain.cpp(70,22): warning C4456: declaration of 'dwValue' hides previous local declaration [C:\a\1\s\src\host\exe\Host.EXE.vcxproj]
       C:\a\1\s\src\host\exe\exemain.cpp(60,11): message : see declaration of 'dwValue' [C:\a\1\s\src\host\exe\Host.EXE.vcxproj]

EDIT: looks like it was fixed with a merge from inbox.

Yeah there was a kerfluffle where someone not in our team stealth changed console without telling us and this is just the fallout of discovering that and reconciling it. Should be good going forward.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

This is lovely. Let's merge it.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 31, 2021
@ghost
Copy link

ghost commented Mar 31, 2021

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit c094723 into main Mar 31, 2021
@ghost ghost deleted the dev/migrie/xmacros-2 branch March 31, 2021 16:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants