aaron.ballman added inline comments.
================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:66-67
+ }
+ Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+ "s");
+ }
----------------
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
https://reviews.llvm.org/D40787
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
