Page MenuHomePhabricator

[clang-tidy] Added alias llvm-else-after-return.
ClosedPublic

Authored by njames93 on Jun 29 2020, 5:17 PM.

Details

Summary

Added an alias llvm-else-after-return from readability-else-after-return to help enforce one of the llvm coding guidelines.

Diff Detail

Event Timeline

njames93 created this revision.Jun 29 2020, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 5:17 PM

This is supposed to have a parent revision D82824 but for some reason phab won't let me add it

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/llvm-else-after-return.rst
16

Please use single back-ticks for option names and values.

njames93 updated this revision to Diff 274359.Jun 30 2020, 1:15 AM

Address doc backticks

njames93 marked an inline comment as done.Jun 30 2020, 1:27 AM
aaron.ballman added inline comments.Jun 30 2020, 5:49 AM
clang-tools-extra/docs/clang-tidy/checks/llvm-else-after-return.rst
4

Hmmm, this means users have five seconds to read the text below before they get redirected. We usually just have the short text about the check being an alias (the first paragraph) and then put the remainder of the information in the target check if it's important.

njames93 marked an inline comment as done.Jun 30 2020, 6:14 AM
njames93 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/llvm-else-after-return.rst
4

Fair point I'll put the extra information in the target check

aaron.ballman accepted this revision.Jun 30 2020, 7:20 AM

Other than the documentation and settling on the option name in the parent revision, I think this LGTM (the changes will be mechanical and can be done without further review once the parent commit is in, unless you want more eyes on it).

This revision is now accepted and ready to land.Jun 30 2020, 7:20 AM
njames93 updated this revision to Diff 274476.Jun 30 2020, 7:43 AM

Moved docs into target check.

Other than the documentation and settling on the option name in the parent revision, I think this LGTM (the changes will be mechanical and can be done without further review once the parent commit is in, unless you want more eyes on it).

Are you happy with the reduced warnings in the this alias, when running locally over llvm I noticed a lot of warnings about else after return where condition variables are used. usually of the format

if (llvm::Expected<T> X = ... ) {
  return *X;
}
else {
  handle(X.takeError());
}

Other than the documentation and settling on the option name in the parent revision, I think this LGTM (the changes will be mechanical and can be done without further review once the parent commit is in, unless you want more eyes on it).

Are you happy with the reduced warnings in the this alias, when running locally over llvm I noticed a lot of warnings about else after return where condition variables are used. usually of the format

if (llvm::Expected<T> X = ... ) {
  return *X;
}
else {
  handle(X.takeError());
}

I can see arguments either way on this. Personally, I would not want the check to diagnose this code because that would encourage people to widen the scope and lifetime of X or require them to introduce a new compound scope to get the same behavior, and I think that's more problematic than the else following a return. I am not certain if others feel the same way though.

I can see arguments either way on this. Personally, I would not want the check to diagnose this code because that would encourage people to widen the scope and lifetime of X or require them to introduce a new compound scope to get the same behavior, and I think that's more problematic than the else following a return. I am not certain if others feel the same way though.

I'll leave this for a bit before merging and see if anyone else wants a weigh in on the default llvm behaviour as this will likely have an effect on other contributors

This revision was automatically updated to reflect the committed changes.