[PATCH] D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel added inline comments.


================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:66-67
+    }
+    Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+                                       "s");
+  }
----------------
aaron.ballman wrote:
> koldaniel wrote:
> > aaron.ballman wrote:
> > > Same concerns here as with the previous review: This fixit can break code if the code disallows narrowing conversions. e.g.,
> > > ```
> > > bool b{std::uncaught_exception()};
> > > ```
> > > In this case, the fixit will take the deprecated code and make it ill-formed instead. Perhaps a better fix-it would be to transform the code into (std::uncaught_exceptions() > 0) to keep the resulting expression type a bool and still not impact operator precedence. Alternatively, you can identify the narrowing conversion case and skip the fix-it entirely in that case (only warn).
> > > 
> > > This example should be a test case.
> > If the fix-it would be a transformation to std::uncaught_exceptions() > 0, the code will break in some cases, for example in case of function pointers or template instantiations. Narrowing conversions would not lead to errors, functionality of the code would remain the same.
> > If the fix-it would be a transformation to std::uncaught_exceptions() > 0, the code will break in some cases, for example in case of function pointers or template instantiations.
> 
> Very true, this check encompasses more than call expressions, which was another concern of mine. For instance, the function pointer case will also result in the fix-it breaking code. Further, it may change SFINAE results (though changes in SFINAE contexts are less of a concern).
> 
> > Narrowing conversions would not lead to errors, functionality of the code would remain the same.
> 
> Incorrect; the narrowing conversion will lead to an error depending on the context. https://godbolt.org/g/v8tCWM
Fair point, which confused me is that there is no such issue when compiling the code with gcc instead of clang. In this case, I think the way forward would be to separate the AST-matchers, and apply a transformation like you proposed when needed.


https://reviews.llvm.org/D40787