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

[njames93]

Summary

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

[njames93]

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

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/llvm-else-after-return.rst

16

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

[njames93]

Address doc backticks

[aaron.ballman]

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]

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]

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).

[njames93]

Moved docs into target check.

[njames93]

In D82825#2122789, @aaron.ballman wrote:

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());
}

[aaron.ballman]

In D82825#2123125, @njames93 wrote:

In D82825#2122789, @aaron.ballman wrote:

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.

[njames93]

In D82825#2123186, @aaron.ballman wrote:

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

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

 //===--- LLVMTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../readability/ElseAfterReturnCheck.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "../readability/QualifiedAutoCheck.h"
 #include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
 #include "PreferIsaOrDynCastInConditionalsCheck.h"
 #include "PreferRegisterOverUnsignedCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm_check {
 
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<readability::ElseAfterReturnCheck>(
+        "llvm-else-after-return");
     CheckFactories.registerCheck<LLVMHeaderGuardCheck>("llvm-header-guard");
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
     CheckFactories.registerCheck<readability::NamespaceCommentCheck>(
         "llvm-namespace-comment");
     CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>(
         "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     CheckFactories.registerCheck<PreferRegisterOverUnsignedCheck>(
         "llvm-prefer-register-over-unsigned");
     CheckFactories.registerCheck<readability::QualifiedAutoCheck>(
         "llvm-qualified-auto");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     Options.CheckOptions["llvm-qualified-auto.AddConstToQualified"] = "0";
+    Options.CheckOptions["llvm-else-after-return.WarnOnUnfixable"] = "0";
+    Options.CheckOptions["llvm-else-after-return.RefactorConditionVariables"] =
+        "0";
     return Options;
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<LLVMModule> X("llvm-module",
                                                   "Adds LLVM lint checks.");
 
 } // namespace llvm_check
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the LLVMModule.
 volatile int LLVMModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-dcl51-cpp
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 - New alias :doc:`cert-str34-c
   <clang-tidy/checks/cert-str34-c>` to
   :doc:`bugprone-signed-char-misuse
   <clang-tidy/checks/bugprone-signed-char-misuse>` was added.
 
+- New alias :doc:`llvm-else-after-return
+  <clang-tidy/checks/llvm-else-after-return>` to
+  :doc:`readability-else-after-return
+  <clang-tidy/checks/readability-else-after-return>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`readability-else-after-return
   <clang-tidy/checks/readability-else-after-return>` check now supports a 
   `WarnOnConditionVariables` option to control whether to refactor condition
   variables where possible.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table:: Aliases..
    `hicpp-use-auto <hicpp-use-auto.html>`_, `modernize-use-auto <modernize-use-auto.html>`_, "Yes"
    `hicpp-use-emplace <hicpp-use-emplace.html>`_, `modernize-use-emplace <modernize-use-emplace.html>`_, "Yes"
    `hicpp-use-equals-default <hicpp-use-equals-default.html>`_, `modernize-use-equals-default <modernize-use-equals-default.html>`_, "Yes"
    `hicpp-use-equals-delete <hicpp-use-equals-delete.html>`_, `modernize-use-equals-delete <modernize-use-equals-delete.html>`_, "Yes"
    `hicpp-use-noexcept <hicpp-use-noexcept.html>`_, `modernize-use-noexcept <modernize-use-noexcept.html>`_, "Yes"
    `hicpp-use-nullptr <hicpp-use-nullptr.html>`_, `modernize-use-nullptr <modernize-use-nullptr.html>`_, "Yes"
    `hicpp-use-override <hicpp-use-override.html>`_, `modernize-use-override <modernize-use-override.html>`_, "Yes"
    `hicpp-vararg <hicpp-vararg.html>`_, `cppcoreguidelines-pro-type-vararg <cppcoreguidelines-pro-type-vararg.html>`_,
+   `llvm-else-after-return <llvm-else-after-return.html>`_, `readability-else-after-return <readability-else-after-return.html>`_, "Yes"
    `llvm-qualified-auto <llvm-qualified-auto.html>`_, `readability-qualified-auto <readability-qualified-auto.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/llvm-else-after-return.rst

+.. title:: clang-tidy - llvm-else-after-return
+.. meta::
+   :http-equiv=refresh: 5;URL=readability-else-after-return.html
+
+llvm-else-after-return
+======================
+
+The llvm-else-after-return check is an alias, please see
+`readability-else-after-return <readability-else-after-return.html>`_
+for more information.
+

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

 .. option:: WarnOnConditionVariables
 
    When `true`, The check will attempt to refactor a variable defined inside
    the condition of the ``if`` statement that is used in the ``else`` branch
    defining them just before the ``if`` statement. This can only be done if 
    the ``if`` statement is the last statement in its parents scope.
    Default value is `true`.
 
+
+LLVM alias
+----------
+
+There is an alias of this check called llvm-else-after-return.
+In that version the options :option:`WarnOnUnfixable` and 
+:option:`WarnOnConditionVariables` are both set to `false` by default.
+
+This check helps to enforce this `LLVM Coding Standards recommendation
+<https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return>`_.