Changed macro constants to constexpr variables #270

[JeanPhilippeKernel]

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.

[BillyONeal]

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

[JeanPhilippeKernel]

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]

Existing stuff seems to be _Ugly

[BillyONeal]

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

[JeanPhilippeKernel]

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

[JeanPhilippeKernel]

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]

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]

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]

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]

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]

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

[StephanTLavavej]

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

[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]

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

[StephanTLavavej]

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.

(To find the comments, I used git diff -U0 master..HEAD | findstr define to get a list of the macros you removed, then I used VSCode to case-sensitive regex-search for \b(BASE|BASE_MAX|EPOCH|MAX_LOCK|MAXSIG|MSEC_PER_SEC|NATS|NDIG|NITEMS|NSEC_PER_MSEC|NSEC_PER_SEC|NSEC_PER_USEC|NSEC100_PER_MSEC|NSEC100_PER_SEC|NSTDSTR|WIN_TICKS_FROM_EPOCH|WIN_TICKS_PER_SECOND)\b.)

[StephanTLavavej]

Thanks for modernizing this code!