This is an archive of the discontinued LLVM Phabricator instance.

[Support] Remove LLVM_ATTRIBUTE_NORETURN
ClosedPublic

Authored by gAlfonso-bit on Jul 27 2021, 11:15 AM.

Details

Reviewers
MaskRay
jhenderson
Group Reviewers
Restricted Project
Commits
rG09529892b518: [Support] Remove LLVM_ATTRIBUTE_NORETURN
Summary

Code should use C++11 [[noreturn]] or C11 _Noreturn instead.

Diff Detail

Event Timeline

gAlfonso-bit created this revision.Jul 27 2021, 11:15 AM
gAlfonso-bit requested review of this revision.Jul 27 2021, 11:15 AM
gAlfonso-bit retitled this revision from [LLVM] Add C++ 14 and C11 noreturn specifier to [LLVM] Use C++ 14 and C11 noreturn specifier.Jul 27 2021, 11:17 AM
This comment was removed by gAlfonso-bit.

I would like to see you undo all the formatting changes.
They are not germane for this patch, and (IMHO) reduce the readability of the code.

This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.

Done!

Thank you. That's *much* easier to review.

I am not an approver for LLVM, but this LGTM.

This comment was removed by gAlfonso-bit.
gAlfonso-bit retitled this revision from [LLVM] Use C++ 14 and C11 noreturn specifier to [LLVM][NFC] Use C++ 14 and C11 noreturn specifier.Jul 27 2021, 2:16 PM
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 6:02 PM
This comment was removed by gAlfonso-bit.
MaskRay requested changes to this revision.EditedJul 27 2021, 6:53 PM

[[noreturn]] is C++11.
In Oct 2016, the minimum compiler requirement was raised to GCC 4.8/MSVC 2015.
We can use [[noreturn]] since then.

I migrated lld/. I will watch the build bots and migrate llvm/

This revision now requires changes to proceed.Jul 27 2021, 6:53 PM
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
Herald added a project: Restricted Project. · View Herald Transcript
gAlfonso-bit retitled this revision from [LLVM][NFC] Use C++ 14 and C11 noreturn specifier to [LLVM][NFC] Remove LLVM_ATTRIBUTE_NORETURN and use [[noreturn]] directly.Jul 28 2021, 7:20 AM
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
MaskRay added a comment.EditedJul 28 2021, 9:39 AM

I migrated all llvm/ files. You need to remove them from the patch.

It is a good idea to drop the macro definition in llvm/include/llvm/Support/Compiler.h separately.

It makes it easy to bring back the definition if some important downstream users want extended time for migration.
(Note: the policy is that downstream is on their own. Our thinking of this is kindness, not an obligation of llvm-project.)
For example, swift is using LLVM_ATTRIBUTE_NORETURN.
(I researched a bit: LLVM_ATTRIBUTE_NORETURN is not common in downstream projects.)

This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
MaskRay accepted this revision.Jul 28 2021, 1:29 PM

Drop llvm/include/llvm/Support/Compiler.h change.

This revision is now accepted and ready to land.Jul 28 2021, 1:29 PM
gAlfonso-bit retitled this revision from [LLVM][NFC] Remove LLVM_ATTRIBUTE_NORETURN and use [[noreturn]] directly to [LLVM][NFC] Replace LLVM_ATTRIBUTE_NORETURN with [[noreturn]].

Removed Compiler.h changes

This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This comment was removed by gAlfonso-bit.
This revision was landed with ongoing or failed builds.Jul 29 2021, 10:10 AM
This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [LLVM][NFC] Replace LLVM_ATTRIBUTE_NORETURN with [[noreturn]] to [Support] Remove LLVM_ATTRIBUTE_NORETURN.Jul 29 2021, 10:11 AM
MaskRay edited the summary of this revision. (Show Details)

Still not passing (what is going on?)

If you click a harbomaster URI, sometimes the builds are good

x64 debian passed
x64 windows passed

and sometimes there may be failures apparently unrelated to your patch.
Some discretion is needed.

I ended up making the refactoring by myself. Your change included some whole-file clang-format formatting which wasn't suitable.
I usually run git diff -U0 --no-color 'HEAD^' | clang/tools/clang-format/clang-format-diff.py -i -p1 to only format related lines.

This comment was removed by gAlfonso-bit.