Replace some macros with constexpr #3362

skyline75489 · 2019-10-29T02:24:11Z

Summary of the Pull Request

Replace some macros with modern constexpr.

References

#2941

PR Checklist

Closes #xxx
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

Macros are just very 1980s and should be used less.

I'm not sure which one is preferred, auto or concrete type.

Validation Steps Performed

Not needed.

skyline75489 · 2019-10-29T02:28:35Z

src/host/_stream.h

                                            _Inout_opt_ PSHORT psScrollY);

-#define LOCAL_BUFFER_SIZE 100
+constexpr auto LOCAL_BUFFER_SIZE = 100;


This is used only in _stream.cpp. I have no idea why it's in the header file.

great question. I'd be fine moving it.

I just checked the history. The reason is historical artifact behind _stream.h (there wasn't a _stream.cpp in the past and it was a weird double-compiled thing for Western v. Eastern languages). I'm OK with this.

zadjii-msft

This overall looks reasonable to me. I think there's been some push to change all constants to PascalCase, (@adiviness can cite the standard I'm thinking of) but this is goodness even without doing that.

zadjii-msft · 2019-10-29T12:12:18Z

src/host/screenInfo.hpp

-#define BOTTOM_LEFT_CORNER 4
-#define BOTTOM_RIGHT_CORNER 5
+
+    static constexpr auto UPPER_LEFT_CORNER = 0;


Maybe we should file a follow-up work item to turn this into a proper enum, instead of whatever this is.

(moved to #3366)

The standard I was trying to apply was that macros would be SCARY_CAPS and constexprs would be PascalCase.

@j4james has a PR that just gets rid of this layer of indirection, which seems fine to me. #3371

DHowett-MSFT · 2019-10-30T20:27:12Z

src/host/_stream.h

                                            _Inout_opt_ PSHORT psScrollY);

-#define LOCAL_BUFFER_SIZE 100
+constexpr auto LOCAL_BUFFER_SIZE = 100;


great question. I'd be fine moving it.

DHowett-MSFT · 2019-10-30T20:28:04Z

src/host/screenInfo.hpp

-#define BOTTOM_LEFT_CORNER 4
-#define BOTTOM_RIGHT_CORNER 5
+
+    static constexpr auto UPPER_LEFT_CORNER = 0;


@j4james has a PR that just gets rid of this layer of indirection, which seems fine to me. #3371

src/interactivity/win32/windowtheme.cpp

skyline75489 · 2019-10-31T01:20:25Z

Updated the PR in favor of #3371.

skyline75489 · 2019-10-31T01:21:10Z

src/terminal/adapter/adaptDispatch.hpp

 #include "terminalOutput.hpp"
 #include <math.h>

-#define XTERM_COLOR_TABLE_SIZE (256)


This is duplicate of

terminal/src/inc/conattrs.hpp

Line 71 in 94fc40e

constexpr WORD XTERM_COLOR_TABLE_SIZE = 256;

skyline75489 · 2019-10-31T01:23:08Z

src/host/settings.hpp


 // To prevent invisible windows, set a lower threshold on window alpha channel.
-#define MIN_WINDOW_OPACITY 0x4D // 0x4D is approximately 30% visible/opaque (70% transparent). Valid range is 0x00-0xff.
+constexpr unsigned int MIN_WINDOW_OPACITY = 0x4D; // 0x4D is approximately 30% visible/opaque (70% transparent). Valid range is 0x00-0xff.


Should this be WORD? See that's how much confusion #define caused us.

And by the way do we prefer WORD over unsigned short?

WORD is just a Windows-ism for unsigned short. I don't know strictly why other than Windows invented a lot of Windows-isms over the years. We've slowly been moving things to the more standard naming instead of the Windows-isms, so feel free to use unsigned short instead.

skyline75489 · 2019-11-01T00:20:21Z

Updated the PR. For the naming part, there are too many rule-breaking constexpr like in

terminal/src/inc/conattrs.hpp

Line 68 in 94fc40e

constexpr COLORREF INVALID_COLOR = 0xffffffff;

I think it's fair save the naming thing for future tasks.

miniksa · 2019-11-01T17:32:28Z

Updated the PR. For the naming part, there are too many rule-breaking constexpr like in

terminal/src/inc/conattrs.hpp

Line 68 in 94fc40e

constexpr COLORREF INVALID_COLOR = 0xffffffff;

I think it's fair save the naming thing for future tasks.

Agreed, feel free to file a follow-on issue for it.

Supports #2941 Previous PRs: #3362 #3416

skyline75489 commented Oct 29, 2019

View reviewed changes

zadjii-msft approved these changes Oct 29, 2019

View reviewed changes

j4james mentioned this pull request Oct 29, 2019

Get rid of the SCREEN_INFORMATION::LineChar array #3371

Merged

5 tasks

DHowett-MSFT reviewed Oct 30, 2019

View reviewed changes

skyline75489 force-pushed the fix/replaceMacroWithConstexpr1 branch from 52a563d to c8c35ca Compare October 31, 2019 01:19

skyline75489 commented Oct 31, 2019

View reviewed changes

Replace some macros with constexpr

d1312b5

skyline75489 force-pushed the fix/replaceMacroWithConstexpr1 branch from c8c35ca to d1312b5 Compare November 1, 2019 00:17

miniksa approved these changes Nov 1, 2019

View reviewed changes

miniksa merged commit 86e0ea7 into microsoft:master Nov 1, 2019

skyline75489 mentioned this pull request Nov 2, 2019

Replace macros with constexpr part 2 #3416

Merged

5 tasks

skyline75489 deleted the fix/replaceMacroWithConstexpr1 branch November 2, 2019 08:54

skyline75489 mentioned this pull request Mar 23, 2021

Prefer constexpr over const across the codebase #9587

Merged

ghost pushed a commit that referenced this pull request Mar 29, 2021

Prefer constexpr over const across the codebase (#9587)

806d992

Supports #2941 Previous PRs: #3362 #3416

Replace some macros with constexpr #3362

Replace some macros with constexpr #3362

Uh oh!

Conversation

skyline75489 commented Oct 29, 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!

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!

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!

Uh oh!

skyline75489 commented Oct 31, 2019

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!

skyline75489 commented Nov 1, 2019

Uh oh!

miniksa commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skyline75489 commented Oct 29, 2019 •

edited

Loading