Skip to content

Conversation

@barcharcraz
Copy link
Contributor

@barcharcraz barcharcraz commented Apr 2, 2020

This adds /Zc:preprocessor to each of our main test matrices. In an effort to avoid adding addtional test configurations I've tacked it on to existing configurations in each matrix to cover the latest and oldest language modes, and IDL=0/2. There is no coverage of release mode since debug is more likely to break.

I have verified that the tests fail with the code from #662

@barcharcraz barcharcraz requested a review from a team as a code owner April 2, 2020 23:23
@CaseyCarter
Copy link
Contributor

FYI, the test failures are because the Windows SDK triggers C5105 warnings (which is largely why the conforming preprocessor isn't yet the default). I assume we'll need to suppress them with /wd5105 for now.

@CaseyCarter CaseyCarter added the test Related to test code label Apr 3, 2020
@StephanTLavavej
Copy link
Member

@CaseyCarter noted that in the near future, /std:c++latest is expected to imply /Zc:preprocessor, so we should alter all /std:c++latest configurations to explicitly enable or disable the new preprocessor.

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.

  1. Can you describe your rationale for which configurations are being altered - i.e. what kind of coverage you want? (e.g. ensuring that we cover IDL=0/2, CXX=14/17/20)
  2. Also, why aren't more matrices being updated, like the native_matrix? Those exist because some tests are incompatible with clr etc. but the conformant preprocessor is still interesting for them. Basically, for all of the matrices in tests/std/tests, I'd like to see them updated, or a rationale why not.
  3. We test libcxx in the most conformant mode available - this implies that tests/libcxx/usual_matrix.lst should now be updated to use /Zc:preprocessor.

@StephanTLavavej
Copy link
Member

Still need libcxx changes.

(At one point, we were trying to keep tr1's configurations in sync with std's, but they might have diverged significantly - in any event, I think there's little value to be gained by changing tr1 here.)

@cbezault cbezault self-requested a review May 9, 2020 04:45
@barcharcraz
Copy link
Contributor Author

Still need libcxx changes.

(At one point, we were trying to keep tr1's configurations in sync with std's, but they might have diverged significantly - in any event, I think there's little value to be gained by changing tr1 here.)

for some reason I thought we only ran libcxx with clang-cl, which, in hindsight, makes zero sense. Change made

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Looks good modulo the issue to track removing /wd5105.

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.

I also observe that the concepts_matrix isn't being modified, but since it's used by a fair number of modern tests, I think it should be added. (The other matrices are either inapplicable or old and obscure.)

@barcharcraz
Copy link
Contributor Author

I also observe that the concepts_matrix isn't being modified, but since it's used by a fair number of modern tests, I think it should be added. (The other matrices are either inapplicable or old and obscure.)

fixed

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Approve with persnickety suggestion to reformat the comments.

@StephanTLavavej StephanTLavavej merged commit fca8493 into microsoft:master May 30, 2020
@StephanTLavavej
Copy link
Member

Thanks for improving this test coverage and making it easier to write conformant code! 😺

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

Labels

test Related to test code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants