Page MenuHomePhabricator

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

Authored by koldaniel on Dec 4 2017, 8:05 AM.

Diff Detail

Event Timeline

koldaniel created this revision.Dec 4 2017, 8:05 AM
xazax.hun added inline comments.Dec 4 2017, 8:22 AM
test/clang-tidy/modernize-replace-uncaught-exception.cpp
41 ↗(On Diff #125344)

Why is this call not ambiguous (global namespace's functions vs std's)?

JonasToth retitled this revision from Replace the usage of std::uncaught_exception with std::uncaught_exceptions to [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions.Dec 4 2017, 8:31 AM
JonasToth added a reviewer: alexfh.
JonasToth added inline comments.
clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp
20 ↗(On Diff #125344)

Could that be a local variable in registerMatchers? Even though its static and const it might be best to reduce its scope as much as possible.

32 ↗(On Diff #125344)

Interesting. Did you consider callExpr as well? I never used declRefExpr in this context but it might be a good choice too. Would it catch taking a function pointer to std::uncaught_exception too? If so, i need to overthink some of my code :)

45 ↗(On Diff #125344)

The implicit assumption here is that one of both must have been matched which is true now.
But adding an assert on U might still be a good thing. You never known how the code evolves and what bugs might come alive.

That just ensures that there is no possible way to dereference a nullptr.

59 ↗(On Diff #125344)

Could the location not simply be EndLoc?

koldaniel added inline comments.Dec 4 2017, 9:45 AM
clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp
32 ↗(On Diff #125344)

According to the original concept callExpr was used, but to match template instantiations and other usages than calling the function directly, it seemed to me a better and simpler solution to use declRefExpr. In case of function pointer initializations like the this:

bool (*foo)();
foo = &std::uncaught_exception;
res = foo();

there will be a warning and the fix will be applied too.

59 ↗(On Diff #125344)

As I could see, when EndLoc was used in Diag (diag(..) of CreateInsertion(...) methods, it still pointed to the beginning of the token marking the whole call. So if the CreateInsertion function looked like this:

Diag << FixItHint::CreateInsertion(EndLoc.getLocWithOffset(TextLength), "s");

had the same effect after applying the fix its as

Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength), "s");

for calls like 'uncaught_exception()' (without std:: namespace written at the beginning, because it increases TextLength, and in case of EndLoc the offset will be counted from the beginning of the function name, not the namespace identifier).

test/clang-tidy/modernize-replace-uncaught-exception.cpp
41 ↗(On Diff #125344)

Global namespace's function will be hidden because of the using declaration, it can be called via ::uncaught_exception(). The call would be ambiguous in case of using namespace:

bool uncaught_exception()
{
    return true;
}
int main()
{
    std::cout<<"std: "<<std::uncaught_exception()<<" own: "<<uncaught_exception()<<std::endl;
    using namespace std;
    std::cout<<"after using: "<<uncaught_exception()<<std::endl; // error: call of overloaded ‘uncaught_exception()’ is ambiguous
}

I think will be good idea to rename check to modernize-use-uncaught-exceptions to be consistent with other similar checks.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

JonasToth added inline comments.Dec 4 2017, 11:23 AM
clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp
32 ↗(On Diff #125344)

Could you add testcases for function pointers please?

Having tests for the template instantiations (in which form? using std::uncaught_exceptions as template argument for callables?) would be a good thing as well.

59 ↗(On Diff #125344)

Thats odd. You could do a Replacement with getSourceRange probably. Sounds more inefficient, but might be shorter in Code.

aaron.ballman added inline comments.Dec 4 2017, 12:23 PM
clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp
59 ↗(On Diff #125344)

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?

xazax.hun added inline comments.Dec 8 2017, 6:30 AM
clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp
59 ↗(On Diff #125344)

Good point, but this kind of fix it is a bit ugly. Maybe skipping the fixit in narrowing cases or only generate the more complicated replacement in the narrowing case would be nicer.

test/clang-tidy/modernize-replace-uncaught-exception.cpp
16 ↗(On Diff #125344)

It would be great to have a test where the template parameter is a function pointer and it is called with uncaught_exception. And with a check fixes make sure that the definition of the template is untouched.

aaron.ballman added inline comments.Dec 8 2017, 12:21 PM
clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp
59 ↗(On Diff #125344)

I would be fine with only using the uglier version if there's a narrowing conversion, or removing the fixit entirely in the narrowing case.

koldaniel updated this revision to Diff 128390.Jan 1 2018, 10:14 AM
aaron.ballman added inline comments.Jan 1 2018, 10:34 AM
clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
25

You might as well make this a std::string rather than const char * because the hasName() matcher accepts that datatype (removes a few implicit converting constructor calls).

62

Comments are full sentences (with correct capitalization and punctuation).

67–68

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.

clang-tidy/modernize/UseUncaughtExceptionsCheck.h
20

warn for the occurrences of -> warn on calls to
replace it -> replace them with calls to

21

Since C++17... -> std::uncaught_exception was deprecated in C++17.

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst
7–9

Same wording suggestions here as above.

koldaniel added inline comments.Jan 1 2018, 10:53 AM
clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
67–68

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.

aaron.ballman added inline comments.Jan 1 2018, 11:19 AM
clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
67–68

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

koldaniel added inline comments.Jan 1 2018, 12:10 PM
clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
67–68

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.

koldaniel updated this revision to Diff 130905.Jan 22 2018, 8:32 AM
aaron.ballman added inline comments.Jan 22 2018, 12:07 PM
clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
45

Can remove spurious parens.

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst
7

Backtick the use of std::uncaught_exception.

65

Please add the newline to the end of the file.

test/clang-tidy/modernize-use-uncaught-exceptions.cpp
43

This is not ideal (the implicit conversion here would do the correct thing).

45

I'd like to see this, and the other examples that require it, moved into its own function body (to segregate it from cases we don't want to see the diagnostics).

64

Applying this fix will break the code so that it no longer compiles.

68

This fix seems bad. If the user accepts the fix, then the code will diagnose because there's no longer a matching call to doSomething2().

koldaniel added inline comments.Jan 24 2018, 10:32 AM
test/clang-tidy/modernize-use-uncaught-exceptions.cpp
64

True, declaration of foo should be changed from type bool to int too. Since as I can see this could cause a lot of problems, shouldn't be there only a warning without fixits?

68

Same type error as earlier, should a fix be applied (changing the type of the parameter in template definition would be unlucky too, maybe a wrapper function which could be passed to doSomething2() and does the conversion) or only a warning?

aaron.ballman added inline comments.Jan 24 2018, 11:16 AM
test/clang-tidy/modernize-use-uncaught-exceptions.cpp
64

I think the right approach here is to warn and not suggest a fix-it.

68

Similarly, I would only warn here as well, and not suggest a fix-it.

koldaniel updated this revision to Diff 132595.Feb 2 2018, 7:57 AM
aaron.ballman accepted this revision.Feb 3 2018, 6:58 AM

This LGTM with a few minor nits to fix.

clang-tidy/modernize/ModernizeTidyModule.cpp
80–81

Please keep this list alphabetized.

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst
20

The indentation of the code examples (here and below) is incorrect.

This revision is now accepted and ready to land.Feb 3 2018, 6:58 AM
koldaniel updated this revision to Diff 133814.Feb 12 2018, 2:14 AM
This revision was automatically updated to reflect the committed changes.