[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D43120#1005100, @Szelethus wrote:

> I also came up with this problem:
>
>    RegularException funcReturningExceptioniTest(int i) {
>      return RegularException();
>    }
>   
>    void returnedValueTest() {
>      funcReturningExceptioniTest(3); //Should this emit a warning?
>   }
>
>
> I'm not sure whether it'd be a good idea to warn about these cases. Unused return values can be found by many other means, and I'm afraid the standard library is filled with these cases.


I think it's fine to not warn on this. It can be caught by other means, but if those turn out to be insufficient for some reason, we can address them in a follow-up patch.



================
Comment at: clang-tidy/misc/ThrowKeywordMissingCheck.cpp:45
+  diag(TemporaryExpr->getLocStart(),
+       "exception instantiated but not bound (did you intend to 'throw'?)");
+}
----------------
Szelethus wrote:
> aaron.ballman wrote:
> > I'm not keen on the wording here ("bound" isn't a common phrase for exceptions). How about `"suspicious exception object created but not thrown; did you mean 'throw %0'"` and then pass in the text for the object construction?
> I tried using the `Lexer::getSourceText` function, but it doesn't print the right parantheses (ex. `RegularException(` instead of `RegularException()`).
> 
> Also, are you sure it'd be a good idea to pass the entire statement there? In cases where it would be long, it could draw the programmer's attention away from the actual problem, the missing `throw` keyword.
That's a reasonable point, I'm fine with the way things are in this patch now.


================
Comment at: docs/ReleaseNotes.rst:70
+
+  Warns if a ``throw`` keyword is potentialy missing before a temporary exception object.
+
----------------
This wording is a bit unclear. How about:

Diagnoses when a temporary object that appears to be an exception is constructed but not thrown.


================
Comment at: test/clang-tidy/misc-throw-keyword-missing.cpp:33
+
+} //namespace std
+
----------------
Missing space after the comment introducer. This happens in other places as well -- you should run the patch through clang-format to be sure to catch all the formatting issues.


================
Comment at: test/clang-tidy/misc-throw-keyword-missing.cpp:54
+  if (i > 0)
+    //CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious exception object created but not thrown; did you mean 'throw {{.*}}'? [misc-throw-keyword-missing]
+    std::runtime_error("Unexpected argument");
----------------
Starting with this warning message and each subsequent message, you can drop most of the wording. e.g., `:[[@LINE+1]]:5: warning: suspicious exception` is sufficient.


================
Comment at: test/clang-tidy/misc-throw-keyword-missing.cpp:132
+
+RegularException funcReturningExceptioniTest(int i) {
+  return RegularException();
----------------
typo: Exceptioni


https://reviews.llvm.org/D43120