Skip to content

Conversation

@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Mar 27, 2020

  • re-clang-formats the tree to comply with new clang 10 clang-format
  • updates our agents to F series VMs which are faster and cheaper for our build workloads
  • defaults scale set to 0 VMs and lets the Azure Pipelines service control all resizing
  • fix tests to pass with the new compilers
  • adds antivirus exclusions for the build directory

Co-authored by Casey Carter and Curtis Bezault

@BillyONeal BillyONeal requested a review from a team as a code owner March 27, 2020 07:49
@BillyONeal BillyONeal self-assigned this Mar 27, 2020
@BillyONeal BillyONeal added the infrastructure Related to repository automation label Mar 27, 2020
@miscco
Copy link
Contributor

miscco commented Mar 27, 2020

So for me I assume that these are exclusively formatting changes in the other files and all functional changes between clang-9 / clang-10 will only come at a later stage?

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO AMAZING

Comment on lines 1527 to 1530
& noexcept(noexcept( //
_Call_front_binder(_Seq{}, _Mypair._Get_first(), _Mypair._Myval2, _STD forward<_Unbound>(_Unbargs)...))) //
-> decltype( //
_Call_front_binder(
_Seq{}, _Mypair._Get_first(), _Mypair._Myval2, _STD forward<_Unbound>(_Unbargs)...)) { //
-> decltype( //
_Call_front_binder(_Seq{}, _Mypair._Get_first(), _Mypair._Myval2, _STD forward<_Unbound>(_Unbargs)...)) { //
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty comments were being used here to improve clang-format's behavior. If you remove them, does clang-format 10 do something reasonable-looking now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admittedly didn't even read what clang-format did :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I'm for, to read the output of humans and robots alike, line by repetitive line, until I find the single wrong one.

stl/inc/random Outdated
} else if _CONSTEXPR_IF (_Cx <= UINT_MAX
&& static_cast<_Uint>(_Mx - 1)
<= (UINT_MAX - _Cx)
/ _Ax) { // unsigned int is sufficient to store intermediate calculation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that _CONSTEXPR_IF isn't horribly damaging this formatting, it appears that this comment should be detached to improve readability.

stl/inc/random Outdated
Comment on lines 331 to 332
<= (ULLONG_MAX - _Cx) / _Ax) { // unsigned long long is sufficient to store
// intermediate calculation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also detach comment?

_NODISCARD inline error_condition error_category::default_error_condition(int _Errval) const
noexcept { // make error_condition for error code
_NODISCARD inline error_condition error_category::default_error_condition(
int _Errval) const noexcept { // make error_condition for error code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be detached according to our modern conventions.

_NODISCARD inline error_condition error_code::default_error_condition() const
noexcept { // make error_condition for error code
_NODISCARD inline error_condition
error_code::default_error_condition() const noexcept { // make error_condition for error code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also detach.

template <class _Ty1, class _Ty2>
struct _Swap_cannot_throw : bool_constant<noexcept(swap(_STD declval<_Ty1>(), _STD declval<_Ty2>()))
&& noexcept(swap(_STD declval<_Ty2>(), _STD declval<_Ty1>()))> {
struct _Swap_cannot_throw : bool_constant<noexcept(swap(_STD declval<_Ty1>(), _STD declval<_Ty2>()))&& noexcept(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this is a regression in appearance. Add an empty comment to force wrapping before the logical AND?

stl/inc/variant Outdated
_NODISCARD constexpr size_t index() const
noexcept { // index of the contained alternative or variant_npos if valueless_by_exception
_NODISCARD constexpr size_t
index() const noexcept { // index of the contained alternative or variant_npos if valueless_by_exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detach comment.

@StephanTLavavej StephanTLavavej changed the title Update to Clang 10, cmake 3.17 Update to Clang 10, CMake 3.17 Mar 27, 2020
@BillyONeal
Copy link
Member Author

So for me I assume that these are exclusively formatting changes in the other files and all functional changes between clang-9 / clang-10 will only come at a later stage?

Correct, except I'm going to have to make 2 functional changes to tests to make them pass.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Mar 27, 2020

Correct, except I'm going to have to make 2 functional changes to tests to make them pass.

Fixes for the feature-test macro test are here in this branch. I'm not sure what's going on with the spaceship test; it looks like cl runs are "failing" with exit status 0?

@cbezault
Copy link
Contributor

They're XPASSing. It looks like there's improvement needed in the errors I output from the test harness in that case.

@CaseyCarter
Copy link
Contributor

They're XPASSing. It looks like there's improvement needed in the errors I output from the test harness in that case.

Ah: C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\VC\Tools\MSVC\14.26.28720\bin\HostX64\x86\cl.EXE is Preview 2 - no wonder.

@BillyONeal BillyONeal changed the title Update to Clang 10, CMake 3.17 Update to Clang 10 Mar 28, 2020
@BillyONeal BillyONeal changed the title Update to Clang 10 Update to Clang 10 and Visual Studio 2019 version 16.6p2 Mar 28, 2020
* re-clang-formats the tree to comply with new clang 10 clang-format
* updates our agents to F series VMs which are faster and cheaper for our build workloads
* defaults scale set to 0 VMs and lets the Azure Pipelines service control all resizing
* fix tests to pass with the new compilers

Co-authored by Casey Carter and Curtis Bezault
@BillyONeal BillyONeal merged commit ff83542 into microsoft:master Mar 29, 2020
@StephanTLavavej
Copy link
Member

I observe that it's difficult to review changes when you squash away history 😿

@StephanTLavavej
Copy link
Member

Fortunately everything looks good (GitHub at least remembers which files have been Viewed and are unchanged). Thanks.

@BillyONeal
Copy link
Member Author

I observe that it's difficult to review changes when you squash away history 😿

Sorry, without squashing it was getting awkward to apply to the msvc repo :/

@StephanTLavavej
Copy link
Member

I'm not sure how you port changes from GitHub to MSVC, but I've found that I can port arbitrarily complex histories to MSVC. As long as the GitHub branch is either rebased or merged with the latest master, a simple git diff master WHATEVER_BRANCH > meow.patch is sufficient to create an inherently squashed patch to apply to MSVC.

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Aug 19, 2020
This was skipped by microsoftGH-682 on 2020-04-09. At the time, we were using
VS 2019 16.6 Preview 2 (microsoftGH-645 merged on 2020-03-28). Now we're using
VS 2019 16.8 Preview 1, and I can't reproduce this ICE locally. I ran
x86 and x64, 30 times each, and they all passed. Let's try restoring
this configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Related to repository automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants