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

<functional>: add noreturn for !_HAS_STATIC_RTTI #3882

Merged
merged 2 commits into from Jul 20, 2023

Conversation

ferhatgec
Copy link
Contributor

No description provided.

@ferhatgec ferhatgec requested a review from a team as a code owner July 17, 2023 15:40
@github-actions github-actions bot added this to Initial Review in Code Reviews Jul 17, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in Code Reviews Jul 17, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jul 17, 2023

This is failing code format validation because we require all files in the repo to be clang-formatted (with the latest version included in VS Preview). You can configure your editor to clang-format on save; for VSCode you can use either the Microsoft C/C++ Extension or xaver's clang-format extension, setting

"C_Cpp.clang_format_path": "C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe",

or

"clang-format.executable": "C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe",

respectively, and then opening the repo as a workspace will automatically configure format-on-save for C++ files (due to the repo's .vscode/settings.json).

You can also manually clang-format files before pushing changes, but we recommend against this; missing files is likely this way, and submitting PRs that fail validation consumes CI system resources which are limited.

What clang-format wants to do here looks kind of weird but it isn't worth the effort to suppress.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jul 18, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews Jul 18, 2023
@achabense
Copy link
Contributor

achabense commented Jul 18, 2023

There are another two usages that call abort/terminate to express the function is not supposed to be called, all without [[noreturn]]. As all previously [[noteturn]] functions are of the type call it intentionally to get some noreturn behavior, I think for consistency we should change them as well in this pr, and maybe try to make obvious that they are special than common [[noreturn]] functions.

_STD terminate(); // Very Bad Things

_CSTD abort();

@StephanTLavavej
Copy link
Member

@achabense Thanks! I'd like to handle that in a separate PR, as this PR is very close to the (undocumented) "de minimis" threshold for requiring the CLA to be signed. Would you like to create one?

_Fake_no_copy_callable_adapter already has a big comment explaining its purpose. _Projected_impl's aborting definition is a perma-workaround for modules that could be commented.

@frederick-vs-ja
Copy link
Contributor

I wonder whether it'll be better to use _STL_UNREACHABLE in these cases. _STL_UNREACHABLE possibly allows "smart" compilers to elide the function more completely, as calling the function is always UB.

@achabense
Copy link
Contributor

achabense commented Jul 19, 2023

I wonder whether it'll be better to use _STL_UNREACHABLE in these cases

Or, should we devise a new macro to express the meaning("should not be called at runtime") more exactly?
I have no idea about what should be the best behavior if the function is really called.

@achabense
Copy link
Contributor

achabense commented Jul 19, 2023

Yes, I would make a pr to

I'd like to help, but I think this needs a lot of decisions. I've made an issue(#3888) about this.

For this pr, I think the following version well be better, as it's more readable imo, takes up less lines, and is easer to refactor in the future:

#if _HAS_STATIC_RTTI
    const type_info& _Target_type() const noexcept override {
        return typeid(_Callable);
    }
#else // _HAS_STATIC_RTTI
    [[noreturn]] const type_info& _Target_type() const noexcept override {
        _CSTD abort();
    }
#endif // _HAS_STATIC_RTTI

@StephanTLavavej
Copy link
Member

@achabense

For this pr, I think the following version well be better, as it's more readable imo

That's a reasonable suggestion but I'd like to handle that in a followup PR, again due to the "de minimis" threshold.

@CaseyCarter CaseyCarter removed their assignment Jul 19, 2023
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Jul 19, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jul 20, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 34f86c5 into microsoft:main Jul 20, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Jul 20, 2023
@StephanTLavavej
Copy link
Member

Thanks for this consistency improvement and congratulations on your first microsoft/STL commit! 🚀 😺 🎉

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

5 participants