[clang-tidy] New option for misc-throw-by-value-catch-by-reference

[baloghadamsoftware]

Summary

Catching trivial objects by value is not dangerous but may be inefficient if they are too large. This patch adds an option WarnOnLargeObject to the checker to also warn if such an object is caught by value. An object is considered as "large" if its size is greater than MaxSize which is another option. Default value is the machine word of the architecture (size of the type size_t).

[torbjoernk]

docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

35–46 ↗

(On Diff #199245)

I think it might be worth adding a note on the fallback to sizeof(size_t) if MaxSize is not set or manually set to std::numeric_limits<uint64_t>::max() by the user.

[aaron.ballman]

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

25 ↗

(On Diff #199245)

Missing a full stop at the end of the comment.

docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

37–40 ↗

(On Diff #199245)

large trivial object -> large, trivial object
affects the perofrmance -> affects the performance
using option MaxSize -> using the MaxSize option.

[lebedev.ri]

docs/ReleaseNotes.rst

182–184 ↗

(On Diff #199245)

for check :doc:..<..> check

(repeated 'check' word)

[baloghadamsoftware]

Updated according to the comments.

[lebedev.ri]

Looks good

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

45 ↗

(On Diff #200266)

because

[aaron.ballman]

LGTM (with the typo fix).

clang-tools-extra/trunk/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

 private:
   void diagnoseThrowLocations(const CXXThrowExpr *throwExpr);
   void diagnoseCatchLocations(const CXXCatchStmt *catchStmt,
                               ASTContext &context);
   bool isFunctionParameter(const DeclRefExpr *declRefExpr);
   bool isCatchVariable(const DeclRefExpr *declRefExpr);
   bool isFunctionOrCatchVar(const DeclRefExpr *declRefExpr);
   const bool CheckAnonymousTemporaries;
+  const bool WarnOnLargeObject;
+  uint64_t MaxSize; // No `const` because we have to set it in two steps.
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H

clang-tools-extra/trunk/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 ThrowByValueCatchByReferenceCheck::ThrowByValueCatchByReferenceCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      CheckAnonymousTemporaries(Options.get("CheckThrowTemporaries", true)) {}
+      CheckAnonymousTemporaries(Options.get("CheckThrowTemporaries", true)),
+      WarnOnLargeObject(Options.get("WarnOnLargeObject", false)),
+      // Cannot access `ASTContext` from here so set it to an extremal value.
+      MaxSize(Options.get("MaxSize", std::numeric_limits<uint64_t>::max())) {}
 
 void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) {
   // This is a C++ only check thus we register the matchers only for C++
   if (!getLangOpts().CPlusPlus)
     return;
 
   Finder->addMatcher(cxxThrowExpr().bind("throw"), this);
   Finder->addMatcher(cxxCatchStmt().bind("catch"), this);
if (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
     if (!PT->getPointeeType()->isAnyCharacterType())
       diag(varDecl->getBeginLoc(), diagMsgCatchReference);
   } else if (!caughtType->isReferenceType()) {
     const char *diagMsgCatchReference = "catch handler catches by value; "
                                         "should catch by reference instead";
     // If it's not a pointer and not a reference then it must be caught "by
     // value". In this case we should emit a diagnosis message unless the type
     // is trivial.
-    if (!caughtType.isTrivialType(context))
+    if (!caughtType.isTrivialType(context)) {
       diag(varDecl->getBeginLoc(), diagMsgCatchReference);
+    } else if (WarnOnLargeObject) {
+      // If the type is trivial, then catching it by reference is not dangerous.
+      // However, catching large objects by value decreases the performance.
+
+      // We can now access `ASTContext` so if `MaxSize` is an extremal value
+      // then set it to the size of `size_t`.
+      if (MaxSize == std::numeric_limits<uint64_t>::max())
+        MaxSize = context.getTypeSize(context.getSizeType());
+      if (context.getTypeSize(caughtType) > MaxSize)
+        diag(varDecl->getBeginLoc(), diagMsgCatchReference);
+    }
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
   finds and replaces cases that match the pattern ``var &&
   isa<X>(var)``, where ``var`` is evaluated twice.
 
 - New :doc:`modernize-use-trailing-return-type
   <clang-tidy/checks/modernize-use-trailing-return-type>` check.
 
   Rewrites function signatures to use a trailing return type.
 
+- The :doc:`misc-throw-by-value-catch-by-reference
+  <clang-tidy/misc-throw-by-value-catch-by-reference.rst>` now supports
+  `WarnOnLargeObject` and `MaxSize` options to warn on any large trivial
+  object caught by value.
+
 Improvements to include-fixer
 -----------------------------
 
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

 -------
 
 .. option:: CheckThrowTemporaries
 
    Triggers detection of violations of the rule `Throw anonymous temporaries
    <https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries>`_.
    Default is `1`.
 
+.. option:: WarnOnLargeObject
+
+   Also warns for any large, trivial object caught by value. Catching a large
+   object by value is not dangerous but affects the performance negatively. The
+   maximum size of an object allowed to be caught without warning can be set
+   using the `MaxSize` option.
+   Default is `0`.
+
+.. option:: MaxSize
+
+   Determines the maximum size of an object allowed to be caught without
+   warning. Only applicable if `WarnOnLargeObject` is set to `1`. If option is
+   set by the user to `std::numeric_limits<uint64_t>::max()` then it reverts to
+   the default value.
+   Default is the size of `size_t`.