Changed macro constants to constexpr variables #270 by JeanPhilippeKernel · Pull Request #487 · microsoft/STL

JeanPhilippeKernel · 2020-02-06T23:41:53Z

Description

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.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
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.

This reverts commit 7e5b7b3.

…cro-change

BillyONeal

Perhaps we should make them less SHOUTY given that they're no longer macros?

JeanPhilippeKernel · 2020-02-07T00:10:30Z

hmm yeah I've been think about that too.
am I allow to do that ? if yes there is some naming convention I should follow or just shouty them ?

sylveon · 2020-02-07T00:18:46Z

Existing stuff seems to be _Ugly

BillyONeal · 2020-02-07T00:19:50Z

Correct, it would have to be _Ugly: NSEC_PER_SEC => _Nsec_per_sec.

JeanPhilippeKernel · 2020-02-07T00:23:54Z

oh okay :) I see , let me apply that then

stl/src/xtime.cpp

Co-Authored-By: Casey Carter <cartec69@gmail.com>

JeanPhilippeKernel · 2020-02-07T01:04:16Z

constexpr long _Nsec_per_usec = 1000L;
and
constexpr long _Nsec100_per_msec = _Nsec_per_msec / 100;

are unused, there is some reason to keep them in the file ?

StephanTLavavej

This looks good. I observe that there are several more files with macro constants that could be cleaned up (found by regex-searching for #define \w+\s+\w+ within stl/src/*.cpp in VSCode):

STL/stl/src/excptptr.cpp

Lines 29 to 33 in 30031d3

    
           // Pre-V4 managed exception code 
        
           #define MANAGED_EXCEPTION_CODE 0XE0434F4D 
        
           // V4 and later managed exception code 
        
           #define MANAGED_EXCEPTION_CODE_V4 0XE0434352

STL/stl/src/filesys.cpp

Lines 270 to 272 in 30031d3

    
           #define WIN_TICKS_PER_SECOND 10000000ULL 
        
           #define WIN_TICKS_FROM_EPOCH (((1970 - 1601) * 365 + 3 * 24 + 17) * 86400ULL * WIN_TICKS_PER_SECOND)

STL/stl/src/ios.cpp

Line 10 in 30031d3

#define NSTDSTR 8 // cin, wcin, cout, wcout, cerr, wcerr, clog, wclog

STL/stl/src/iosptrs.cpp

Line 33 in 30031d3

#define NATS 10 // fclose, xgetloc, locks, facet free, etc.

STL/stl/src/xlock.cpp

Line 14 in 30031d3

#define MAX_LOCK 8 // must be power of two

STL/stl/src/xnotify.cpp

Line 12 in 30031d3

#define NITEMS 20

STL/stl/src/xstoflt.cpp

Lines 13 to 15 in 30031d3

    
           #define BASE   10 // decimal 
        
           #define NDIG   9 // decimal digits per long word 
        
           #define MAXSIG (5 * NDIG) // maximum significant digits to keep

STL/stl/src/xstoul.cpp

Line 17 in 30031d3

#define BASE_MAX 36 // largest valid base

STL/stl/src/xstoull.cpp

Line 17 in 30031d3

#define BASE_MAX 36 // largest valid base

STL/stl/src/xstoxflt.cpp

Lines 14 to 16 in 30031d3

    
           #define BASE   16 // hexadecimal 
        
           #define NDIG   7 // hexadecimal digits per long element 
        
           #define MAXSIG (5 * NDIG) // maximum significant digits to keep

STL/stl/src/xwstoflt.cpp

Lines 13 to 15 in 30031d3

    
           #define BASE   10 // decimal 
        
           #define NDIG   9 // decimal digits per long element 
        
           #define MAXSIG (5 * NDIG) // maximum significant digits to keep

STL/stl/src/xwstoxfl.cpp

Lines 14 to 16 in 30031d3

    
           #define BASE   16 // hexadecimal 
        
           #define NDIG   7 // hexadecimal digits per long element 
        
           #define MAXSIG (5 * NDIG) // maximum significant digits to keep

After rereading #270, I realize that I had quoted xtime.cpp intending it to be a non-exhaustive example of the files that needed to be cleaned up, but I didn't actually say that many files were affected. Oops!

If you'd like us to merge this PR as-is, we can certainly do so, and keep #270 open. If you want to extend these changes to more files, we can continue reviewing this PR. Just let us know! :-)

StephanTLavavej · 2020-02-07T01:14:04Z

constexpr long _Nsec_per_usec = 1000L;
and
constexpr long _Nsec100_per_msec = _Nsec_per_msec / 100;

are unused, there is some reason to keep them in the file ?

Great catch! If they're unused, they should definitely be eliminated. (We've changed this file over many years, so it's possible they were used at some point, and we missed these bits.)

BillyONeal · 2020-02-07T01:30:33Z

STL/stl/src/excptptr.cpp

I think we should leave the ones in here alone because that constant needs to match how that's declared in CLR headers.

JeanPhilippeKernel · 2020-02-07T01:35:58Z

Oh so, we can continue reviewing this PR, I'll extend these changes to more files.
I would like to be sure that things to be properly cleaned up :)

JeanPhilippeKernel · 2020-02-07T01:39:26Z

constexpr long _Nsec_per_usec = 1000L;
and
constexpr long _Nsec100_per_msec = _Nsec_per_msec / 100;
are unused, there is some reason to keep them in the file ?

Great catch! If they're unused, they should definitely be eliminated. (We've changed this file over many years, so it's possible they were used at some point, and we missed these bits.)

perfect, so I'll remove them

StephanTLavavej · 2020-02-07T01:41:09Z

Given @BillyONeal's explanation, I agree that leaving excptptr.cpp unchanged is best at this time.

…cro-change

stl/src/filesys.cpp

stl/src/xlock.cpp

StephanTLavavej

Looks good, just found some parentheses that are no longer needed 🎉

stl/src/filesys.cpp

stl/src/xstoflt.cpp

stl/src/xstoxflt.cpp

stl/src/xwstoflt.cpp

stl/src/xwstoxfl.cpp

StephanTLavavej

Looks great - I’ll port this to the Microsoft-internal repo and run a build and test pass. (We’re looking forward to when we finish migrating the build and tests to GitHub 😹)

JeanPhilippeKernel · 2020-02-07T22:11:33Z

Awesome !!! it's a pleasure for me ! =D

StephanTLavavej · 2020-02-07T22:43:16Z

While double-checking the Microsoft-internal port, I noticed a few more comments that needed to be updated. I think I found them all, so I went ahead and pushed a change to your branch. Running the build and tests now.

StephanTLavavej · 2020-02-08T00:06:07Z

Thanks for modernizing this code!

JeanPhilippeKernel added 5 commits February 6, 2020 17:45

Update _MSVC_STL_UPDATE value to February 2020

b0573ef

Updated macro constants to constexpr variables microsoft#270

7e5b7b3

Revert "Updated macro constants to constexpr variables microsoft#270"

59b9474

This reverts commit 7e5b7b3.

Changed macro constants to constexpr variables microsoft#270

91167a0

Merge branch 'master' of https://github.com/microsoft/STL into fix-ma…

44604c6

…cro-change

JeanPhilippeKernel requested a review from a team as a code owner February 6, 2020 23:41

BillyONeal reviewed Feb 6, 2020

View reviewed changes

StephanTLavavej added the enhancement Something can be improved label Feb 7, 2020

updated variables to _Ugly naming convention

eecf828

CaseyCarter approved these changes Feb 7, 2020

View reviewed changes

stl/src/xtime.cpp Outdated Show resolved Hide resolved

Update stl/src/xtime.cpp

c403c18

Co-Authored-By: Casey Carter <cartec69@gmail.com>

StephanTLavavej approved these changes Feb 7, 2020

View reviewed changes

BillyONeal approved these changes Feb 7, 2020

View reviewed changes

StephanTLavavej linked an issue Feb 7, 2020 that may be closed by this pull request

STL: Change macro constants to constexpr variables #270

Closed

JeanPhilippeKernel added 4 commits February 7, 2020 18:03

updated macro change

8551ae8

removed unecesarry comment

fd3f8e8

Merge branch 'master' of https://github.com/microsoft/STL into fix-ma…

0dfcf9f

…cro-change

clang-format updated

bde3b36

CaseyCarter suggested changes Feb 7, 2020

View reviewed changes

stl/src/filesys.cpp Outdated Show resolved Hide resolved

stl/src/xlock.cpp Show resolved Hide resolved

StephanTLavavej requested changes Feb 7, 2020

View reviewed changes

stl/src/filesys.cpp Outdated Show resolved Hide resolved

stl/src/xstoflt.cpp Outdated Show resolved Hide resolved

stl/src/xstoxflt.cpp Outdated Show resolved Hide resolved

stl/src/xwstoflt.cpp Outdated Show resolved Hide resolved

stl/src/xwstoxfl.cpp Outdated Show resolved Hide resolved

removed unnecessary parentheses

e483f6a

JeanPhilippeKernel requested review from CaseyCarter and StephanTLavavej February 7, 2020 20:58

StephanTLavavej self-assigned this Feb 7, 2020

StephanTLavavej approved these changes Feb 7, 2020

View reviewed changes

Update comments.

1eee336

CaseyCarter approved these changes Feb 7, 2020

View reviewed changes

StephanTLavavej merged commit ed3cbf3 into microsoft:master Feb 8, 2020

StephanTLavavej mentioned this pull request Feb 13, 2020

Fix construct tag #497

Merged

4 tasks

	// Pre-V4 managed exception code
	#define MANAGED_EXCEPTION_CODE 0XE0434F4D

	// V4 and later managed exception code
	#define MANAGED_EXCEPTION_CODE_V4 0XE0434352

	#define WIN_TICKS_PER_SECOND 10000000ULL

	#define WIN_TICKS_FROM_EPOCH (((1970 - 1601) * 365 + 3 * 24 + 17) * 86400ULL * WIN_TICKS_PER_SECOND)

	#define BASE 10 // decimal
	#define NDIG 9 // decimal digits per long word
	#define MAXSIG (5 * NDIG) // maximum significant digits to keep

	#define BASE 16 // hexadecimal
	#define NDIG 7 // hexadecimal digits per long element
	#define MAXSIG (5 * NDIG) // maximum significant digits to keep

Conversation

JeanPhilippeKernel commented Feb 6, 2020

Description

Checklist

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

JeanPhilippeKernel commented Feb 7, 2020

Uh oh!

sylveon commented Feb 7, 2020

Uh oh!

BillyONeal commented Feb 7, 2020

Uh oh!

JeanPhilippeKernel commented Feb 7, 2020

Uh oh!

Uh oh!

JeanPhilippeKernel commented Feb 7, 2020

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Feb 7, 2020

Uh oh!

BillyONeal commented Feb 7, 2020

Uh oh!

JeanPhilippeKernel commented Feb 7, 2020

Uh oh!

JeanPhilippeKernel commented Feb 7, 2020

Uh oh!

StephanTLavavej commented Feb 7, 2020

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

JeanPhilippeKernel commented Feb 7, 2020

Uh oh!

StephanTLavavej commented Feb 7, 2020

Uh oh!

StephanTLavavej commented Feb 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants