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

Various cleanups: extern "C" #4143

Merged
merged 7 commits into from Nov 7, 2023

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Nov 2, 2023

We've accumulated inconsistency between using our _EXTERN_C macro and directly saying extern "C":

STL/stl/inc/memory

Lines 3720 to 3723 in f392449

_EXTERN_C
_CRTIMP2_PURE void __cdecl _Lock_shared_ptr_spin_lock() noexcept;
_CRTIMP2_PURE void __cdecl _Unlock_shared_ptr_spin_lock() noexcept;
_END_EXTERN_C

extern "C" {
void __cdecl __sanitizer_annotate_contiguous_container(
const void* _First, const void* _End, const void* _Old_last, const void* _New_last) noexcept;
}

The macro used to be important when some of our headers were dual C and C++. Now that they're always C++, and that we don't expect to play special games with extern "C" in the foreseeable future, we should resolve this inconsistency and favor dropping the macro.

(In contrast, we're quite disciplined about _STD_BEGIN in product code, and I do want to play special games with it soon.)

  • Replace: _EXTERN_C ... _END_EXTERN_C => extern "C" { ... } // extern "C"
  • Add // extern "C" comments.
  • Wrap stacktrace's function pointer typedefs in extern "C".
    • It doesn't really matter because typedefs are "transparent" (they don't create new types), but I believe it's desirable for the signatures of extern "C" functions to restrict themselves to mentioning only things that are within the extern "C" universe. During the initial modularization effort, I overhauled extern "C" functions to mention only extern "C" structs, where it did matter.
  • sharedmutex.cpp: Move the _Smtx_t typedef within extern "C".
    • Again, it doesn't really matter for typedefs, but this is now consistent with stl/inc/xthreads.h:

      STL/stl/inc/xthreads.h

      Lines 21 to 28 in f392449

      _EXTERN_C
      using _Thrd_id_t = unsigned int;
      struct _Thrd_t { // thread identifier for Win32
      void* _Hnd; // Win32 HANDLE
      _Thrd_id_t _Id;
      };
      using _Smtx_t = void*;
  • Mark _Atomic_memory_order_meow as extern "C" to match vcruntime.
  • Mark _C_meow_complex as extern "C" to match the UCRT.

It doesn't really matter because typedefs are "transparent" (they don't create new types), but I believe it's desirable for the signatures of extern "C" functions to restrict themselves to mentioning only things that are within the extern "C" universe. During the initial modularization effort, I overhauled extern "C" functions to mention only extern "C" structs, where it did matter.
Again, it doesn't really matter for typedefs, but this is now consistent with stl/inc/xthreads.h.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Nov 2, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 2, 2023 20:51
@github-actions github-actions bot added this to Initial Review in Code Reviews Nov 2, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Nov 2, 2023
@StephanTLavavej
Copy link
Member Author

I am a greedy kitty who's speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Nov 6, 2023
@StephanTLavavej
Copy link
Member Author

I've pushed a merge with main to resolve a trivial adjacent-edit conflict with #4150 in __msvc_sanitizer_annotate_container.hpp.

@StephanTLavavej StephanTLavavej merged commit 3f15c7f into microsoft:main Nov 7, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Nov 7, 2023
@StephanTLavavej StephanTLavavej deleted the extern-sea branch November 7, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants