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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
