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

Repeat export specifiers on definitions, so that they stay if the declarations are gone #3927

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Aug 6, 2023

We should have export specifiers on both function declaration and definition.
This is because after some future changes the declaration may be gone - either entirely, or header be no longer included.
By repeating export specifiers we make sure the function type and its impact on export surface stays the same.

See #3912 (comment)

Additionally, this simplifies the declarations in stl/src/xmtx.hpp to match the definitions (the declarations are functionally unchanged).

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner August 6, 2023 09:16
@StephanTLavavej
Copy link
Member

I understand what you're doing here, but can you please provide a PR description so that others can understand (both now, and people in the future doing PR archaeology)? For a PR fixing typos in comments, a title of "Fix typos" with no description would indeed be sufficient, but this PR is doing something much less obvious.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Aug 6, 2023
@AlexGuteniev
Copy link
Contributor Author

Updated.

@AlexGuteniev AlexGuteniev changed the title More declarations! Repeat export specifiers on definitions, so that they stay if the declarations are gone Aug 6, 2023
@StephanTLavavej StephanTLavavej self-assigned this Aug 7, 2023
Handling this variation is exactly what `_MRTIMP2_PURE` and `__CLRCALL_PURE_OR_CDECL` do.
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej
Copy link
Member

I've pushed a merge with main to resolve a conflict in stl/src/xmtx.hpp where #3900 updated preprocessor comments. Resolved by accepting this PR's elimination of the preprocessor logic.

@StephanTLavavej StephanTLavavej merged commit ae01669 into microsoft:main Aug 11, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for fixing more of these inconsistencies and defending against future mistakes! 🛠️ 🛡️ 😸

@AlexGuteniev AlexGuteniev deleted the imp branch August 11, 2023 05:45
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants