Skip to content

Code cleanup: avoid macro-like identifiers, remove double newlines - #865

Merged
Stephan T. Lavavej (StephanTLavavej) merged 11 commits into
microsoft:masterfrom
StephanTLavavej:avoid_macros
May 30, 2020
Merged

Code cleanup: avoid macro-like identifiers, remove double newlines#865
Stephan T. Lavavej (StephanTLavavej) merged 11 commits into
microsoft:masterfrom
StephanTLavavej:avoid_macros

Conversation

@StephanTLavavej

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) commented May 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

The STL uses _Ugly (or __ugly in certain situations) identifiers to avoid colliding with users, especially with user macros. While all _Ugly identifiers are reserved, we conventionally avoid single-letter names like _X because they look too much like macros (being composed entirely of capital letters and underscores). And, notably, _T is macroized by tchar.h.

In #47 (comment), Casey Carter (@CaseyCarter) observed that we still have identifiers like _N0 which also resemble macros, as they contain no lowercase letters. In the past, _M1 and _M2 were specifically problematic:

STL/stl/inc/__msvc_all_public_headers.hpp

Line 20 in 212de15

// VSO-768746: mbctype.h macroizes _MS, _MP, _M1, and _M2. Include it first for test coverage.

We've mostly been avoiding _[A-Z][0-9] identifiers in new code. We should clean up all occurrences in the headers, so that they aren't imitated by new changes.

This PR makes the following changes, where the new names aren't currently used in each file (thus avoiding any potential for conflict):

  • bitset: _E0, _E1 to _Elem0, _Elem1
  • charconv: _U1, _U2 to _Ux1, _Ux2
  • chrono: _T0 to _Tx0
  • random: _R1 to _Rx1 etc.
  • ratio: _R1, _R2 to _Rx1, _Rx2 etc.
    • Also _R2_inverse to _Rx2_inverse, for consistency.
  • regex: _E1, _E2 to _Ex1, _Ex2
    • Also _E0x, _E1x to _Arg0, _Arg1, to avoid visual confusion.
  • xlocmon: _E0 to _Elem0
  • xstring: _N0 to _Nx

In this file, the new name is currently used elsewhere, but not in a conflicting way (totally different scope):

  • memory: _N0 to _Nx

While we're making widespread superficial changes, this removes many occurrences of double newlines that were inconsistent or unnecessary. It doesn't remove all double newlines in headers; some seemed consistently used (although we could drop all of them and enforce it forever via clang-format if we wanted to).

Finally, this detaches a few comments from braces, and unwraps a few lines (oddly, clang-format was neutral - it accepted either the wrapped or unwrapped lines).

@StephanTLavavej
xstring: _N0 to _Nx (no existing occurrences)
20269c7
@StephanTLavavej
bitset: _E0 to _Elem0, _E1 to _Elem1 (no existing occurrences)
ac24a64
@StephanTLavavej
ratio: _D1, _D2, _N1, _N2, _R1, _R2, _R2_inverse to _Dx1, _Dx2, _Nx1,…
c977c63 
… _Nx2, _Rx1, _Rx2, _Rx2_inverse (no existing occurrences)
@StephanTLavavej
random: _R1 to _Rx1 etc. (no existing occurrences)
73673a9
@StephanTLavavej
chrono: _T0 to _Tx0 (no existing occurrences)
5ac1936
@StephanTLavavej
memory: _N0 to _Nx (no conflicts)
9c17f18
@StephanTLavavej
xlocmon: _E0 to _Elem0 (no existing occurrences)
f26c7e0 
diff --git a/stl/inc/xlocmon b/stl/inc/xlocmon
index e5cffea..8b50d0e 100644
--- a/stl/inc/xlocmon
+++ b/stl/inc/xlocmon
@@ -682,12 +682,12 @@ protected:
         }

         const ctype<_Elem>& _Ctype_fac = _STD use_facet>(_Iosbase.getloc());
-        const _Elem _E0                = _Ctype_fac.widen('0');
+        const _Elem _Elem0             = _Ctype_fac.widen('0');

         string_type _Val2(static_cast(_Count), _Elem{});
         _Ctype_fac.widen(_Buf, _Buf + _Count, &_Val2[0]);
-        _Val2.append(_Exp, _E0); // scale by trailing zeros
-        return _Putmfld(_Dest, _Intl, _Iosbase, _Fill, _Negative, _Val2, _E0);
+        _Val2.append(_Exp, _Elem0); // scale by trailing zeros
+        return _Putmfld(_Dest, _Intl, _Iosbase, _Fill, _Negative, _Val2, _Elem0);
     }

     virtual _OutIt __CLR_OR_THIS_CALL do_put(_OutIt _Dest, bool _Intl, ios_base& _Iosbase, _Elem _Fill,
@@ -718,8 +718,9 @@ protected:
     }

 private:
-    _OutIt _Putmfld(_OutIt _Dest, bool _Intl, ios_base& _Iosbase, _Elem _Fill, bool _Neg, string_type _Val,
-        _Elem _E0) const { // put string_type with just digits to _Dest
+    _OutIt _Putmfld(
+        _OutIt _Dest, bool _Intl, ios_base& _Iosbase, _Elem _Fill, bool _Neg, string_type _Val, _Elem _Elem0) const {
+        // put string_type with just digits to _Dest
         const _Mpunct<_Elem>* _Ppunct_fac;
         if (_Intl) {
             _Ppunct_fac = _STD addressof(_STD use_facet<_Mypunct1>(_Iosbase.getloc())); // international
@@ -732,7 +733,7 @@ private:
         const auto _Fracdigits = static_cast(_Ifracdigits < 0 ? -_Ifracdigits : _Ifracdigits);

         if (_Val.size() <= _Fracdigits) {
-            _Val.insert(0, _Fracdigits - _Val.size() + 1, _E0);
+            _Val.insert(0, _Fracdigits - _Val.size() + 1, _Elem0);
         } else if (*_Grouping.c_str() != CHAR_MAX && '\0' < *_Grouping.c_str()) {
             // grouping specified, add thousands separators
             const _Elem _Kseparator = _Ppunct_fac->thousands_sep();
@@ -821,20 +822,17 @@ private:

             case money_base::value: // put value field
                 if (_Fracdigits == 0) {
-                    _Dest = _Put(_Dest, _Val.begin(),
-                        _Val.size()); // no fraction part
+                    _Dest = _Put(_Dest, _Val.begin(), _Val.size()); // no fraction part
                 } else if (_Val.size() <= _Fracdigits) { // put leading zero, all fraction digits
-                    *_Dest++ = _E0;
+                    *_Dest++ = _Elem0;
                     *_Dest++ = _Ppunct_fac->decimal_point();
-                    _Dest    = _Rep(_Dest, _E0,
-                        _Fracdigits - _Val.size()); // insert zeros
+                    _Dest    = _Rep(_Dest, _Elem0, _Fracdigits - _Val.size()); // insert zeros
                     _Dest    = _Put(_Dest, _Val.begin(), _Val.size());
                 } else { // put both integer and fraction parts
-                    _Dest    = _Put(_Dest, _Val.begin(),
-                        _Val.size() - _Fracdigits); // put integer part
+                    _Dest    = _Put(_Dest, _Val.begin(), _Val.size() - _Fracdigits); // put integer part
                     *_Dest++ = _Ppunct_fac->decimal_point();
-                    _Dest    = _Put(_Dest, _Val.end() - static_cast(_Fracdigits),
-                        _Fracdigits); // put fraction part
+                    _Dest =
+                        _Put(_Dest, _Val.end() - static_cast(_Fracdigits), _Fracdigits); // put fraction part
                 }
                 break;

@@ -851,16 +849,15 @@ private:
         }

         if (1 < _Sign.size()) {
-            _Dest = _Put(_Dest, _Sign.begin() + 1,
-                _Sign.size() - 1); // put remainder of sign
+            _Dest = _Put(_Dest, _Sign.begin() + 1, _Sign.size() - 1); // put remainder of sign
         }

         _Iosbase.width(0);
         return _Rep(_Dest, _Fill, _Fillcount); // put trailing fill
     }

-    static _OutIt _Put(_OutIt _Dest, typename string_type::const_iterator _Source,
-        size_t _Count) { // put [_Source, _Source + _Count) to _Dest
+    static _OutIt _Put(_OutIt _Dest, typename string_type::const_iterator _Source, size_t _Count) {
+        // put [_Source, _Source + _Count) to _Dest
         for (; 0 < _Count; --_Count, (void) ++_Dest, ++_Source) {
             *_Dest = *_Source;
         }
@StephanTLavavej
charconv: _U1, _U2 to _Ux1, _Ux2 (no existing occurrences)
ad567b0
@StephanTLavavej
regex: _E0x, _E1x to _Arg0, _Arg1 (no existing occurrences)
7af7c3c
@StephanTLavavej
regex: _E1, _E2 to _Ex1, _Ex2 (no existing occurrences)
ae03fc6
@StephanTLavavej
Remove many double newlines.
4b45414 
This doesn't remove all double newlines - just ones that
seemed especially unnecessary or inconsistent.
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added the enhancement Something can be improved label May 27, 2020
Merged
@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 40c3f1f into microsoft:master May 30, 2020
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented May 30, 2020

Copy link
Copy Markdown
Member Author

Thanks again for mentioning these identifiers, Casey Carter (@CaseyCarter)! As superficial as they may seem, the charconv macro collision caused real user pain in the past, which this will avoid in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal Billy O'Neal (BillyONeal) BillyONeal approved these changes
+1 more reviewer
@CaseyCarter Casey Carter (CaseyCarter) CaseyCarter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej)

Labels

enhancement Something can be improved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@StephanTLavavej @CaseyCarter @BillyONeal