This is an archive of the discontinued LLVM Phabricator instance.

[-fms-extensions] Make some exception specification warnings/errors compatible with what cl.exe does
ClosedPublic

Authored by akhuang on Dec 23 2021, 3:24 PM.

Details

Summary

Make clang-cl error when a function definition is missing 'noexcept',
and succeed without warnings when missing '__declspec(nothrow)' or 'throw'.

Fixes pr52860

Diff Detail

Unit TestsFailed

Event Timeline

akhuang requested review of this revision.Dec 23 2021, 3:24 PM
akhuang created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2021, 3:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the patch! This looks roughly right to me.

Maybe the list of ESTs that are allowed to be mismatched should be opt-in instead of opt-out? (i.e. instead of checking for "not EST_BasicNoexcept / EST_DependentNoexcept", check for EST_NoThrow (I think?) Not sure which way is better.

(Looks like the old code was added for https://llvm.org/PR25265)

Could you check that we still emit the warning on line 5 in https://godbolt.org/z/bxfx8jsjd ? The test is mostly from https://docs.microsoft.com/en-us/cpp/build/reference/zc-noexcepttypes?view=msvc-170 -- it feels like we might want to have different behavior in c++17 (and later) and c++14 (and earlier) for some of the diags, possibly. Looks like MSVC also has an error on line 15 by default (with /std:c++17, it seems to accept it with /std:c++14), while we only warn.

(I'm a bit surprised the noexcept bit doesn't make it into the mangled name in the ms abi, but we're consistent with msvc about this so that's all good.)

clang/lib/Sema/SemaExceptionSpec.cpp
397

nit: LLVM style says "no else after return", so this should become a regular if now.

akhuang updated this revision to Diff 397760.Jan 5 2022, 5:48 PM
akhuang marked an inline comment as done.

Fix warning behavior

Thanks for the patch! This looks roughly right to me.

Maybe the list of ESTs that are allowed to be mismatched should be opt-in instead of opt-out? (i.e. instead of checking for "not EST_BasicNoexcept / EST_DependentNoexcept", check for EST_NoThrow (I think?) Not sure which way is better.

(Looks like the old code was added for https://llvm.org/PR25265)

Could you check that we still emit the warning on line 5 in https://godbolt.org/z/bxfx8jsjd ? The test is mostly from https://docs.microsoft.com/en-us/cpp/build/reference/zc-noexcepttypes?view=msvc-170 -- it feels like we might want to have different behavior in c++17 (and later) and c++14 (and earlier) for some of the diags, possibly. Looks like MSVC also has an error on line 15 by default (with /std:c++17, it seems to accept it with /std:c++14), while we only warn.

(I'm a bit surprised the noexcept bit doesn't make it into the mangled name in the ms abi, but we're consistent with msvc about this so that's all good.)

It did not still emit the warning but I've changed it now. I'm not sure why https://llvm.org/PR25265 added a new warning (ext_ms_missing_exception_specification) instead of just using ext_missing_exception_specification; I guess I'll get rid of it anyway.

thakis added inline comments.Jan 6 2022, 6:50 AM
clang/test/SemaCXX/MicrosoftCompatibility.cpp
4

Could you add a run line with -std=c++17?

akhuang updated this revision to Diff 397930.Jan 6 2022, 10:21 AM
akhuang marked an inline comment as done.

Add std=c++17 to the test.

thakis accepted this revision.Jan 6 2022, 6:30 PM

Thanks!

This revision is now accepted and ready to land.Jan 6 2022, 6:30 PM
This revision was landed with ongoing or failed builds.Jan 7 2022, 2:42 PM
This revision was automatically updated to reflect the committed changes.