Refactor: Simplify boolean expressions in lib/AST

[LegalizeAdulthood]

Summary

Simplify boolean expressions using true and false with clang-tidy

[majnemer]

lib/AST/ASTContext.cpp

8002–8005

There should be braces around the return to match the braces in the blocks.

lib/AST/DeclBase.cpp

497–498

Please put braces around this.

[LegalizeAdulthood]

lib/AST/ASTContext.cpp

8002–8005

Fixed and run through clang-format

lib/AST/DeclBase.cpp

497–498

Fixed and run through clang-format

[LegalizeAdulthood]

Added braces and run through clang-format

[rjmccall]

Much like the Parser patch, I don't think these changes are actually an improvement. In both cases, we have a long series of clauses, each of which returns true or false. The motivation for the patch appears to be purely that it's logically equivalent to directly return the last condition. So what? It doesn't make the code any more readable. It jumbles the last comment. It makes the last clause visually inconsistent with the earlier clauses. It makes it more awkward to adjust or reorder clauses. Why would we want to do this? There has to be some reason beyond "a lint tool told us we could simplify it this way".

[LegalizeAdulthood]

Thanks for your feedback John, this is very helpful in improving this check.

In D8532#145845, @rjmccall wrote:

[...] Why would we want to do this?

So basically you're saying the tool is reporting a false positive for code that is of the form if (e) return true; else return false; when this code fragment appears as the last in a series if if/else if clauses?

When it is a single instance of that code fragment, you don't have a problem with it?

If that understanding is correct, then I can see that this might be a matter of style or preference. This is a readability check, after all, and most things in readability have some aspect of a subjective viewpoint.

If the default behavior were to not consider such clauses when they are at the end of a larger if/else clause and there was an option for including those cases as well, would that address the issue?

[rjmccall]

In D8532#145849, @LegalizeAdulthood wrote:

Thanks for your feedback John, this is very helpful in improving this check.

In D8532#145845, @rjmccall wrote:

[...] Why would we want to do this?

So basically you're saying the tool is reporting a false positive for code that is of the form if (e) return true; else return false; when this code fragment appears as the last in a series if if/else if clauses?

In my opinion, yes.

When it is a single instance of that code fragment, you don't have a problem with it?

Right. I have no problem with turning "if (<condition>) { return true; } else { return false; }" into "return <condition>;". It's really the chained use that's kindof suspect to me.

I guess my argument is that, if you've already got more than one interesting condition requiring special behavior, it's better to be consistent across the entire if/else chain, especially because having added cases in the past means it's more likely that you'll add more cases in the future. If you've only got one condition, it's better to be more compact until it proves inadequate.

If that understanding is correct, then I can see that this might be a matter of style or preference. This is a readability check, after all, and most things in readability have some aspect of a subjective viewpoint.

If the default behavior were to not consider such clauses when they are at the end of a larger if/else clause and there was an option for including those cases as well, would that address the issue?

That would completely address my objection, yes, thank you.

[LegalizeAdulthood]

Thanks for your ideas, John, I'm going to incorporate this into the clang-tidy check.

[djasper]

I don't feel strongly about this, and I can see some of your reasoning.
However, an "if (a) return true; else return false;" is very suspect to me
and I think "return a;" is more readable, independent of whether it is at
the end of a chain or not.

However, there is an underlying issue here. The existing code is violating
LLVM's coding standard of "don't use else after return" (
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return).
So in these chains, all of the "else"s should be removed.

[LegalizeAdulthood]

In D8532#145854, @djasper wrote:

I don't feel strongly about this, and I can see some of your reasoning.
However, an "if (a) return true; else return false;" is very suspect to me
and I think "return a;" is more readable, independent of whether it is at
the end of a chain or not.

This is my view on readability as well and one of the reasons I wrote this check for clang-tidy.

These patches arose from me trying out the new check on a "real codebase" instead of my single FileCheck based test file. A couple of issues arose from applying the new check on the code that is helping me to improve my check beyond just handling the basics, so this is all great feedback for me, even if the patches don't get accepted.

The LLVM coding rule of "no else after a return, continue, etc." probably should be turned into a clang-tidy check, at least for detection if not correction.

[LegalizeAdulthood]

Abandoning this changeset after improving the clang-tidy check to ignore simplification of conditional return statements at the end of a chain of conditional statements.

lib/AST/ASTContext.cpp

if (!VD->isFileVarDecl())
       return false;
     // Global named register variables (GNU extension) are never emitted.
     if (VD->getStorageClass() == SC_Register)
       return false;
   } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     // We never need to emit an uninstantiated function template.
     if (FD->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate)
       return false;
-  } else if (isa<OMPThreadPrivateDecl>(D))
-    return true;
-  else
-    return false;
+  } else {
+    return isa<OMPThreadPrivateDecl>(D);
+  }
 
   // If this is a member of a class template, we do not need to emit it.
   if (D->getDeclContext()->isDependentContext())
     return false;
 
   // Weak references don't produce any output by themselves.
   if (D->hasAttr<WeakRefAttr>())
     return false;
 

lib/AST/DeclBase.cpp

bool Decl::canBeWeakImported(bool &IsDefinition) const {
   } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(this)) {
     if (FD->hasBody()) {
       IsDefinition = true;
       return false;
     }
     return true;
 
   // Objective-C classes, if this is the non-fragile runtime.
-  } else if (isa<ObjCInterfaceDecl>(this) &&
-             getASTContext().getLangOpts().ObjCRuntime.hasWeakClassImport()) {
-    return true;
-
-  // Nothing else.
   } else {
-    return false;
+    return isa<ObjCInterfaceDecl>(this) &&
+           getASTContext().getLangOpts().ObjCRuntime.hasWeakClassImport();
   }
 }
 
 bool Decl::isWeakImported() const {
   bool IsDefinition;
   if (!canBeWeakImported(IsDefinition))
     return false;
 
   for (const auto *A : attrs()) {
     if (isa<WeakImportAttr>(A))