Fix construct tag by JeanPhilippeKernel · Pull Request #497 · microsoft/STL

JeanPhilippeKernel · 2020-02-12T18:21:12Z

Description

Fixes #468.

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

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

…cro-change

…ft#468)

…format This reverts commit f4aa09f.

StephanTLavavej · 2020-02-13T03:47:54Z

stl/inc/codecvt

+constexpr int _Little_first   = 1;
+constexpr int _Big_first      = 2;
+constexpr int _Bytes_per_word = 4;


In stl/inc headers, we conventionally say _INLINE_VAR constexpr, for example:

STL/stl/inc/algorithm

Line 30 in 2bd2bd2

_INLINE_VAR constexpr int _ISORT_MAX = 32; // maximum size for insertion sort

This expands to inline constexpr in C++17 mode, which makes a difference if the constant is ever address-taken (hopefully not for these constants, but we follow the convention anyways).

StephanTLavavej · 2020-02-13T04:09:27Z

stl/inc/codecvt

-#define _BYTES_PER_WORD 4
+constexpr int _Little_first   = 1;
+constexpr int _Big_first      = 2;
+constexpr int _Bytes_per_word = 4;


_BYTES_PER_WORD appears to have been completely unused and should be eliminated.

StephanTLavavej · 2020-02-13T04:16:31Z

stl/inc/xmemory

+}; // tag to indicate that a proxy is being allocated before it is safe to bind to a
+   // _Container_base12


This comment can now be unwrapped to a single line. (clang-format wraps excessively long comments, but won't unwrap in situations like this.)

StephanTLavavej · 2020-02-13T04:24:00Z

stl/inc/xutility

-struct contiguous_iterator_tag : random_access_iterator_tag {};
+struct contiguous_iterator_tag : random_access_iterator_tag {
+    explicit contiguous_iterator_tag() = default;
+};


Unfortunately, the "explicit default constructor" improvement can't be extended to these Standard types, so please revert this group of changes.

The issue is that WG21-N4849 [std.iterator.tags]/1 (see https://eel.is/c++draft/std.iterator.tags#1 for HTML instead of PDF) specifies the Standard tag types, with implicit default constructors, so we have to match that observable behavior. In contrast, we can do basically whatever we want with internal non-Standard types like _Priority_tag below.

Oh okay let's revert so !
thanks a lot !! I didn't aware about that

StephanTLavavej · 2020-02-13T04:33:50Z

stl/src/memory_resource.cpp

-    extern "C" _CRT_SATELLITE_1 _Aligned_new_delete_resource_impl* __cdecl _Aligned_new_delete_resource() noexcept {
+    _EXTERN_C
+    _CRT_SATELLITE_1 _Aligned_new_delete_resource_impl* __cdecl _Aligned_new_delete_resource() noexcept {


The extern "C" changes in this file are unrelated to the tag changes in the rest of the PR. While this transformation doesn't change the meaning of the code, it introduces inconsistency with the declarations in <memory_resource>. If you believe that we should consistently rely on _EXTERN_C blocks for declarations and definitions, please file an enhancement issue so we can make a decision - and then if we decide on a convention, it can be cleaned up in a separate PR.

The general principle is that large code transformations/cleanups should be performed in separate PRs for ease of code reviewing and history reading. Mixing in very small changes can be OK (e.g. comment changes, or the _Default_resource{nullptr} initialization change above - that one is at least thematically related to "brace-initialize more things").

I noticed similar changes in #487 but didn't object then because they were small and increased consistency between the declarations and definitions. I probably should have mentioned my concerns though, apologies for not doing so earlier.

Yeah totally right !, let's revert and consider another PR for these changes

The extern "C" changes in this file are unrelated to the tag changes in the rest of the PR. While this transformation doesn't change the meaning of the code, it introduces inconsistency with the declarations in <memory_resource>. If you believe that we should consistently rely on _EXTERN_C blocks for declarations and definitions, please file an enhancement issue so we can make a decision - and then if we decide on a convention, it can be cleaned up in a separate PR.

you don't need to apologies 😉, what do you think about a file an enhancement issue and assign it to me ?

I noticed similar changes in #487 but didn't object then because they were small and increased consistency between the declarations and definitions. I probably should have mentioned my concerns though, apologies for not doing so earlier.

StephanTLavavej · 2020-02-13T04:39:53Z

stl/src/parallel_algorithms.cpp

 }

-extern "C" {
+EXTERN_C_START


These are <Windows.h> macros, but we conventionally use the STL's macros for this (in fact, I had to look up where this was coming from). I think it would be fine to use the STL macros here in this PR.

this means that we should include <yvals_core.h>
as we said above, let's file a new enhancement issue for a diff PR

StephanTLavavej · 2020-02-13T04:40:20Z

stl/src/parallel_algorithms.cpp

 }

-} // extern "C"
+EXTERN_C_END // extern "C"


When changing this to the STL's macro, the comment should also be dropped (as it no longer serves a purpose).

StephanTLavavej · 2020-02-13T04:40:39Z

stl/src/sharedmutex.cpp


-extern "C" {
-
+EXTERN_C_START


Same comments about using STL macros instead of <Windows.h> macros.

revert "explicit default constructor" on Standard tag type, revert "extern "C"" for another PR

StephanTLavavej

Perfect; I will port this to our Microsoft-internal repo and merge it soon. Thank you!

`_Lex_compare_optimize` now has an explicit default constructor, so we can't return `{}`. Usage through an alias template also needs to be fixed.

BillyONeal · 2020-02-29T06:58:36Z

/azp run

azure-pipelines · 2020-02-29T06:58:43Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2020-02-29T22:22:49Z

Thank you for improving the codebase's consistency and readability!

JeanPhilippeKernel and others added 16 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

updated variables to _Ugly naming convention

eecf828

Update stl/src/xtime.cpp

c403c18

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

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

removed unnecessary parentheses

e483f6a

Update comments.

1eee336

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

72c9caa

…cro-change

update tag

8c2e1b2

Consistently use empty braces to construct tags like _Meow{} (microso…

71d78c2

…ft#468)

JeanPhilippeKernel requested a review from a team as a code owner February 12, 2020 18:21

JeanPhilippeKernel added 4 commits February 12, 2020 18:31

revert change

0f890ba

fix clang-format for vector and xmemory

f4aa09f

Revert "fix clang-format for vector and xmemory" and re-update files …

102c248

…format This reverts commit f4aa09f.

update files

3e1903b

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

StephanTLavavej requested changes Feb 13, 2020

View reviewed changes

Files revision to apply _INLINE_VAR constexpr convention,

f51241b

revert "explicit default constructor" on Standard tag type, revert "extern "C"" for another PR

JeanPhilippeKernel requested a review from StephanTLavavej February 13, 2020 13:36

StephanTLavavej self-assigned this Feb 19, 2020

StephanTLavavej mentioned this pull request Feb 26, 2020

STL: Avoid declaring multiple variables on a single line #522

Closed

StephanTLavavej approved these changes Feb 28, 2020

View reviewed changes

BillyONeal approved these changes Feb 29, 2020

View reviewed changes

Fix compiler errors.

4690500

`_Lex_compare_optimize` now has an explicit default constructor, so we can't return `{}`. Usage through an alias template also needs to be fixed.

StephanTLavavej added 2 commits February 29, 2020 01:59

Replace more _LITTLE_FIRST and _BIG_FIRST.

f8b03b5

Within stdext::cvt, use _STD qualification.

4ae0835

StephanTLavavej merged commit 577827a into microsoft:master Feb 29, 2020

		}; // tag to indicate that a proxy is being allocated before it is safe to bind to a
		// _Container_base12

Conversation

JeanPhilippeKernel commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

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!

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!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Feb 29, 2020

Uh oh!

azure-pipelines bot commented Feb 29, 2020

Uh oh!

StephanTLavavej commented Feb 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JeanPhilippeKernel commented Feb 12, 2020 •

edited

Loading