Page MenuHomePhabricator

[clang-tidy] readability-else-after-return: also diagnose noreturn function calls.
Needs RevisionPublic

Authored by lebedev.ri on Dec 20 2017, 10:14 AM.

Details

Summary

The readability-else-after-return check was not warning about an else after a call to the function that will not return. In particular, marker with a noreturn attribute, be it either C++11 [[noreturn]], or the __attribute__((noreturn)).
This differential adds support to diagnose normal function calls, and calls to member functions; but not constructors, destructors and special member functions.

A follow-up for D40505.

Diff Detail

Event Timeline

lebedev.ri created this revision.Dec 20 2017, 10:14 AM

Depends on D41455.
I'm open to suggestions re diagnostic wording.
The current 'noreturn call' seems lazy, and is basically a placeholder.

aaron.ballman added inline comments.Dec 20 2017, 12:00 PM
clang-tidy/readability/ElseAfterReturnCheck.cpp
43

You've spotted the downside to this clever construct -- it is hard to make the wording not terrible here. :-)

I think the diagnostic should read do no use 'else' after a call to a function that does not return. This is trickier because the current %0 is always quoted.

docs/ReleaseNotes.rst
189

This seems to be > 80 cols, so please reformat. Also, it should be "that require cleanups and calls to functions marked with the noreturn attribute" (fixes a few grammatical issues at once).

This check could also handle else after goto.

This check could also handle else after goto.

Yes, certainly. Though i'm not too sure on the restrictions.
The obvious precondition is, the label can to be defined anywhere except after the goto in the same (or child) compound statement:

if(condition) {
  goto label:
  label: ; // <- false-positive
} else ...
alexfh requested changes to this revision.Mar 23 2018, 8:18 AM

The problem with this proposed functionality is that it's not always obvious at the call site whether a function is noreturn. exit() may be an easy case, but there may be much less obvious names and whether the function is noreturn may depend on compile-time configuration or be instantiation-dependent in template code (https://godbolt.org/g/fNtpCy):

template<typename T>
struct X {
  void f();
};
template<>
struct X<int> {
    [[noreturn]] void f();
};
template<typename T>
void f(int x) {
  if (x) {
    X<T>().f();
  } else {  // Should the check complain here?
    //...
  }
}
This revision now requires changes to proceed.Mar 23 2018, 8:18 AM