Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and simplify the feature-test macro test #4103

Merged

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Oct 19, 2023

  • Drop extra spaces in #error messages.
  • Mechanically simplify.
    • This test originally covered both the compiler and library feature-test macros, and was therefore extremely paranoid about checking the behavior of the preprocessor. We gave the compiler test coverage to the compiler team a long time ago, so we can simplify the library test coverage now. This commit is a 100% regex search-and-replace, from:
      #ifndef (__cpp_lib_\w+)
      #error \1 is not defined
      #elif \1 != (\d{6}L)
      #error \1 is not \2
      #else
      STATIC_ASSERT\(\1 == \2\);
      #endif
      to:
      #ifdef $1
      STATIC_ASSERT($1 == $2);
      #else
      #error $1 is not defined
      #endif
      except with \n between each line of the "from" and "to" (presented here on separate lines for clarity).
  • Simplify __cpp_lib_chrono and __cpp_lib_shared_ptr_arrays.
    • These are always defined, but to varying values, so they use a slightly different pattern.
  • Simplify __cpp_lib_shift.
    • This was trying to be extra helpful with a message saying "is OLD_VALUE when it should be NEW_VALUE", but that made the code verbose and unsystematic. This test is passing, so we don't need elaborate error messages in case of failure.
  • Fix damaged __cpp_lib_containers_ranges test.
    • This would never have detected __cpp_lib_containers_ranges being defined outside of the relevant mode.
  • Mechanically simplify again (100% regex).
    • From:
      #ifdef (__cpp_lib_\w+)
      (STATIC_ASSERT\(\1 == \d{6}L\);)
      #else
      #error \1 is not defined
      #endif
      to:
      $2
      
  • Manually simplify __cpp_lib_chrono and __cpp_lib_shared_ptr_arrays again.
    • This removes all occurrences of "is not defined".
  • Mechanically simplify yet again (100% regex).
    • From:
      #else
      #ifdef (__cpp_lib_\w+)
      #error \1 is defined
      #endif
      #endif
      to:
      #elif defined($1)
      #error $1 is defined
      #endif
  • Manually simplify __cpp_lib_shared_timed_mutex.
    • Testing for non-/clr:pure mode allows this to follow the consistent pattern.
  • Fix damaged __cpp_lib_is_layout_compatible and __cpp_lib_is_pointer_interconvertible tests.
    • These tests weren't doing anything for C++14/17 mode. Now they'll detect the macros being present when they shouldn't be.
  • DROP SHOUTY COMMENT.

This test originally covered both the compiler and library feature-test macros, and was therefore extremely paranoid about checking the behavior of the preprocessor.
We gave the compiler test coverage to the compiler team a long time ago, so we can simplify the library test coverage now.

This commit is a 100% regex search-and-replace, from:

    #ifndef (__cpp_lib_\w+)
    #error \1 is not defined
    #elif \1 != (\d{6}L)
    #error \1 is not \2
    #else
    STATIC_ASSERT\(\1 == \2\);
    #endif

to:

    #ifdef $1
    STATIC_ASSERT($1 == $2);
    #else
    #error $1 is not defined
    #endif

except with \n between each line of the "from" and "to" (presented here on separate lines for clarity).
These are always defined, but to varying values, so they use a slightly different pattern.
This was trying to be extra helpful with a message saying "is OLD_VALUE when it should be NEW_VALUE", but that made the code verbose and unsystematic. This test is passing, so we don't need elaborate error messages in case of failure.
This would never have detected `__cpp_lib_containers_ranges` being defined outside of the relevant mode.
@StephanTLavavej StephanTLavavej added the test Related to test code label Oct 19, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 19, 2023 00:31
@StephanTLavavej

This comment was marked as resolved.

@CaseyCarter

This comment was marked as resolved.

@CaseyCarter

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

From:

    #ifdef (__cpp_lib_\w+)
    (STATIC_ASSERT\(\1 == \d{6}L\);)
    #else
    #error \1 is not defined
    #endif

to:

    $2
…` again.

This removes all occurrences of "is not defined".
From:

    #else
    #ifdef (__cpp_lib_\w+)
    #error \1 is defined
    #endif
    #endif

to:

    #elif defined($1)
    #error $1 is defined
    #endif
Testing for non-`/clr:pure` mode allows this to follow the consistent pattern.
…r_interconvertible` tests.

These tests weren't doing anything for C++14/17 mode. Now they'll detect the macros being present when they shouldn't be.
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.

This is so much nicer - thanks!

@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2023
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit adea8d5 into microsoft:main Oct 20, 2023
@StephanTLavavej StephanTLavavej deleted the feature-test-macro-test branch October 20, 2023 22:50
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.

2 participants