Enable /Zc:preprocessor #10593

lhecker · 2021-07-09T00:19:55Z

This commit is a preparation for upcoming changes to KeyChordSerialization for #7539 and #10203.
In order to support variadic macros, /Zc:preprocessor was enabled, which required changing unrelated parts of the project.

PR Checklist

I work here
Tests added/passed

Validation Steps Performed

Project still compiles ✔️

lhecker · 2021-07-12T19:15:23Z

src/buffer/out/TextAttribute.hpp

@@ -212,7 +212,7 @@ constexpr bool operator!=(const TextAttribute& a, const TextAttribute& b) noexce
 #ifdef UNIT_TESTING

 #define LOG_ATTR(attr) (Log::Comment(NoThrowString().Format( \
-    L#attr L"=%s", VerifyOutputTraits<TextAttribute>::ToString(attr).GetBuffer())))
+    L## #attr L"=%s", VerifyOutputTraits<TextAttribute>::ToString(attr).GetBuffer())))


L#foo used to turn it into a L"foo" string, no matter what kind of literal foo was.
With standard conform macros this doesn't work anymore and we have to cast foo to a string using #foo and then concatenate it with L which forms L## #foo.

lhecker · 2021-07-12T19:17:34Z

src/cascadia/TerminalSettingsEditor/Utils.h

+    {                                                                                                                                                       \
+        std::vector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> name##List;                                                                    \
+        _##name##Map = winrt::single_threaded_map<enumType, winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>();                                     \
+        auto enumMapping##name = winrt::Microsoft::Terminal::Settings::Model::EnumMappings::enumMappingsName();                                             \


I can only suggest turning on white space suppressing in the GitHub diff viewer (append ?w=1 to the current url).

In standard conform macros ## can only be used to concatenate valid symbols together. You can't concatenate with ::. This change fixes the macro, but also ensures that local variables are eagerly released to the allocator by using a do-while scope.

Curious: why use do { ... } while(0); instead of just { ... }? Wouldn't that accomplish the same thing of adding a scope so that the variabels are released?

The do-while loop has the advantage that it cannot be used in any other statement, for instance within the conditional statement of a while loop or if condition. It forces the programmer to treat the macro as a regular statement that has no return value and that prevents additional misuse.

lhecker · 2021-07-12T19:20:12Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+        return sub && isProfilesDefaultsOrigin(sub.SourceProfile());
+    };
+
+#define DUPLICATE_SETTING_MACRO(settingName)                                                         \


These macros were both invalid. Since I had to touch them anyways I fixed their COM overhead as a "drive by". We now use generic lambdas to abstract away as much work as possible from the macro. This doesn't just make debugging and reading easier (the macro looks a lot less scary without the diff), but also reduces code sizes after compilation, since the argument to our generic lambdas will always be the same type.

lhecker · 2021-07-12T19:26:22Z

src/inc/WilErrorReporting.h

+                TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage),
+                TraceLoggingLevel(WINEVENT_LEVEL_ERROR),
+                TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance),
+                TraceLoggingStruct(14, "wilResult", "wilResult"),


Due to the strict macros, trailing commas in a macro are now forbidden. These can occur with variadic macros like

#define FOO(...) foo(123, __VA_ARGS__);

If you don't pass any arguments to FOO it'll generate a trailing comma, which isn't valid C++ code. One might think trailing commas should be supported in C++, because why not, but they aren't. Instead there's a hackjob called __VA_OPT__ which lets you write

#define FOO(...) foo(123 __VA_OPT__(,) __VA_ARGS__);

Since MSVC doesn't support the official __VA_OPT__, we have to write ## __VA_ARGS__ from now on, which isn't legit macro syntax, but it works despite using /Zc:preprocessor.

In this case we don't control TraceLoggingStruct, which doesn't use the ## __VA_ARGS__ trick.
As such we have to add a third argument (the "comment") to any TraceLoggingWrite parameters from now on.

I suspect that I can accept this. Thanks.

We can also set it to the empty string explicitly if we want to save on the .rodata segment. That's what used to happen with the previous non-conformant mode anyways (technically).

lhecker · 2021-07-12T19:26:49Z

src/inc/consoletaeftemplates.hpp

-            VERIFY_SUCCEEDED(StringCchCopy(szBuf, ARRAYSIZE(szBuf), L"(ev: "));
+
+            WEX::Common::NoThrowString str;
+            str.Append(L"(ev: ");


This change is not needed. I did it before I realized I had to upgrade the entire TAEF package to make it work.

It's fine if you keep it in my opinion.

src/inc/til/string.h

Replaces `KeyModifiers` with the pretty much equivalent `VirtualKeyModifiers` enum in winrt. After doing this I noticed #10593 which changes the KeyChords a lot, but it seems these PRs are still compatible The issue also mentions replacing Vkey with `Windows::System::VirtualKey`, but I chose not to because that enum only includes a subset of the keys terminal supports here (no VK_OEM_* keys) ## Validation Steps Performed Changed key bind in config, and confirmed it still works after restarting terminal Closes #877

carlos-zamora

Honestly, all of this looks good to me. I think the only thing I found was the duplicate copyright line haha.

That said, this seems like it's a bit too big in scope. I think it'd be nice if you split this into 3 PRs:

Enable /Zc:preprocessor (Variadic Macro thing)
(based off 1)
1. Clean up KeyChordSerialization
2. Add unit tests to til/string

Alternatively, I would be ok with you merging this PR without a squash, then just add a few more details on each commit/endeavor/scenario in the PR body.

@DHowett generally has more opinions regarding the repos commit history haha. Thoughts?

carlos-zamora · 2021-07-12T22:13:12Z

src/cascadia/TerminalSettingsEditor/Utils.h

+    {                                                                                                                                                       \
+        std::vector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> name##List;                                                                    \
+        _##name##Map = winrt::single_threaded_map<enumType, winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>();                                     \
+        auto enumMapping##name = winrt::Microsoft::Terminal::Settings::Model::EnumMappings::enumMappingsName();                                             \


Curious: why use do { ... } while(0); instead of just { ... }? Wouldn't that accomplish the same thing of adding a scope so that the variabels are released?

carlos-zamora · 2021-07-12T22:48:26Z

src/cppwinrt.build.pre.props

-      <AdditionalOptions>%(AdditionalOptions) /permissive- /bigobj /Zc:twoPhase-</AdditionalOptions>
-      <DisableSpecificWarnings>28204;%(DisableSpecificWarnings)</DisableSpecificWarnings>
+      <ConformanceMode>true</ConformanceMode>
+      <UseStandardPreprocessor>true</UseStandardPreprocessor>
+      <AdditionalOptions>%(AdditionalOptions) /bigobj /Zc:twoPhase-</AdditionalOptions>
+      <DisableSpecificWarnings>5104;5105;28204;%(DisableSpecificWarnings)</DisableSpecificWarnings>


This seems fine to me, but somebody with way more knowledge of our build system should double check that this is ok.

FYI the warnings are:

C5104: found 'L#foo' in macro replacement list, did you mean 'L""#foo'?
Which occurs in WRL, which we can't immediately fix. I suggest we migrate everything that uses WRL to WIL over time.

C5105: macro expansion producing 'defined' has undefined behavior
This occurs in winbase.h, and is caused by Windows headers using conditional expressions inside definitions. They first declare something like #define FOO (_WIN32_WINNT >= 0x0502) and then use it as #if FOO, which isn't valid C/C++.

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

carlos-zamora · 2021-07-12T22:54:51Z

src/cascadia/TerminalSettingsModel/KeyChordSerialization.h

 // Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.


carlos-zamora · 2021-07-12T22:55:42Z

src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp

+constexpr std::wstring_view CTRL_KEY{ L"ctrl" };
+constexpr std::wstring_view SHIFT_KEY{ L"shift" };
+constexpr std::wstring_view ALT_KEY{ L"alt" };
+constexpr std::wstring_view WIN_KEY{ L"win" };


Why don't these need to be static?

Because constexpr doesn't make any sense whatsoever. If you put a constexpr in global scope it's behaving as if it was static automatically. I can add static for consistency.

DHowett · 2021-07-13T00:36:32Z

I'd left a comment on one of the commits, but I am not sure it shows up on the PR. Perhaps email?

lhecker · 2021-07-13T02:49:16Z

@DHowett Yes I saw it and fixed the issue. It was about the SCW (?) macro change being wrong which I then reverted.

DHowett · 2021-07-13T21:53:34Z

src/cascadia/TerminalSettingsEditor/Utils.h

-    std::sort(begin(name##List), end(name##List), EnumEntryComparator<enumType>());                                                                     \
-    _##name##List = winrt::single_threaded_observable_vector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>(std::move(name##List));
+#define INITIALIZE_BINDABLE_ENUM_SETTING(name, enumMappingsName, enumType, resourceSectionAndType, resourceProperty)                                        \
+    do                                                                                                                                                      \


thank you for taking the opportunity to fix this!

DHowett · 2021-07-13T21:56:20Z

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

-        com_ptr<DeadPeasant> tombstone;
-        tombstone.attach(new DeadPeasant());
-        m->_peasants[peasantID] = *tombstone;
+        m->_peasants[peasantID] = *winrt::make_self<DeadPeasant>();


technically *make_self<T>() should be equivalent to make<T>() 😄

DHowett · 2021-07-13T21:58:59Z

src/cascadia/inc/cppwinrt_utils.h

-    type _##name{ __VA_ARGS__ };                                                       \
-    void _set##name(const type& value)                                                 \
-    {                                                                                  \
-        const_cast<type&>(_##name) = value;                                            \


oh hey! an old implementation detail we didn't need any more.

TRIVIA: The storage for an "observable" property used to be marked const so that members of the class could not set it without calling _setX which forced them to send a notification every time they changed it.

DHowett · 2021-07-13T22:00:38Z

src/cppwinrt.build.pre.props

+      <ConformanceMode>true</ConformanceMode>
+      <UseStandardPreprocessor>true</UseStandardPreprocessor>
+      <AdditionalOptions>%(AdditionalOptions) /bigobj /Zc:twoPhase-</AdditionalOptions>
+      <DisableSpecificWarnings>5104;5105;28204;%(DisableSpecificWarnings)</DisableSpecificWarnings>


These are fine. They're reports about non-conforming macros that we have to suppress because we turned on the conforming preprocessor.

DHowett · 2021-07-13T22:03:06Z

src/inc/WilErrorReporting.h

+                TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage),
+                TraceLoggingLevel(WINEVENT_LEVEL_ERROR),
+                TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance),
+                TraceLoggingStruct(14, "wilResult", "wilResult"),


I suspect that I can accept this. Thanks.

DHowett · 2021-07-13T22:04:02Z

src/inc/consoletaeftemplates.hpp

-            VERIFY_SUCCEEDED(StringCchCopy(szBuf, ARRAYSIZE(szBuf), L"(ev: "));
+
+            WEX::Common::NoThrowString str;
+            str.Append(L"(ev: ");


It's fine if you keep it in my opinion.

ghost · 2021-07-13T22:39:47Z

Hello @lhecker!

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.

This commit is a preparation for upcoming changes to KeyChordSerialization for #7539 and #10203. In order to support variadic macros, /Zc:preprocessor was enabled, which required changing unrelated parts of the project. * [x] I work here * [x] Tests added/passed * Project still compiles ✔️ (cherry picked from commit 32fbd4c)

mimvdb mentioned this pull request Jul 9, 2021

Use WinRT VirtualKeyModifiers instead of a custom enum #10603

Merged

lhecker force-pushed the dev/lhecker/7539-key-bindings branch from b521529 to a949fd9 Compare July 12, 2021 18:45

lhecker marked this pull request as ready for review July 12, 2021 18:59

lhecker force-pushed the dev/lhecker/7539-key-bindings branch from bf821dd to ffe7273 Compare July 12, 2021 19:03

This comment has been minimized.

Sign in to view

lhecker force-pushed the dev/lhecker/7539-key-bindings branch from ffe7273 to d09c3a2 Compare July 12, 2021 19:13

lhecker commented Jul 12, 2021

View reviewed changes

carlos-zamora reviewed Jul 12, 2021

View reviewed changes

Enable /Zc:preprocessor

9360050

lhecker force-pushed the dev/lhecker/7539-key-bindings branch from 79c0f42 to 9360050 Compare July 13, 2021 21:48

lhecker changed the title ~~Clean up KeyChordSerialization~~ Enable /Zc:preprocessor Jul 13, 2021

DHowett approved these changes Jul 13, 2021

View reviewed changes

carlos-zamora approved these changes Jul 13, 2021

View reviewed changes

Address reviewer comments

9cff9e7

lhecker added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Terminal The new Windows Terminal. AutoMerge Marked for automatic merge by the bot when requirements are met labels Jul 13, 2021

ghost merged commit 32fbd4c into main Jul 13, 2021

ghost deleted the dev/lhecker/7539-key-bindings branch July 13, 2021 23:00

ianjoneill mentioned this pull request Jul 16, 2021

Preliminary work to add Swap Panes functionality (GH Issues 1000, 4922) #10638

Merged

6 tasks

zadjii-msft mentioned this pull request Oct 6, 2021

Re-align the commandline and the VS builds #11446

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable /Zc:preprocessor #10593

Enable /Zc:preprocessor #10593

lhecker commented Jul 9, 2021 •

edited

Loading

This comment has been minimized.

lhecker Jul 12, 2021

lhecker Jul 12, 2021

carlos-zamora Jul 12, 2021

lhecker Jul 13, 2021

lhecker Jul 12, 2021 •

edited

Loading

lhecker Jul 12, 2021

DHowett Jul 13, 2021

lhecker Jul 13, 2021

lhecker Jul 12, 2021

DHowett Jul 13, 2021

carlos-zamora left a comment

carlos-zamora Jul 12, 2021

carlos-zamora Jul 12, 2021

lhecker Jul 13, 2021 •

edited

Loading

carlos-zamora Jul 12, 2021

carlos-zamora Jul 12, 2021

lhecker Jul 13, 2021

DHowett commented Jul 13, 2021

lhecker commented Jul 13, 2021

DHowett Jul 13, 2021

DHowett Jul 13, 2021

DHowett Jul 13, 2021

DHowett Jul 13, 2021

DHowett Jul 13, 2021

DHowett Jul 13, 2021

ghost commented Jul 13, 2021

		// Copyright (c) Microsoft Corporation.
		// Copyright (c) Microsoft Corporation.

Enable /Zc:preprocessor #10593

Enable /Zc:preprocessor #10593

Conversation

lhecker commented Jul 9, 2021 • edited Loading

PR Checklist

Validation Steps Performed

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Jul 13, 2021

lhecker commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 13, 2021

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.

lhecker commented Jul 9, 2021 •

edited

Loading

lhecker Jul 12, 2021 •

edited

Loading

lhecker Jul 13, 2021 •

edited

Loading

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.