[clang-tidy] New checker to replace deprecated throw() specifications

[hintonda]

Summary

This checker allows users to replace deprecated throw()
specifications with noexcept.

It also allows users to specify a user defined macro to be used
instead of noexect, which is useful when supporting older compilers.
However, the macro will not be used for noexcect(false) since that
doesn't make sense.

[hintonda]

• Addressed a few comments for clarity/readability.

[hintonda]

clang-tidy/modernize/UseNoexceptCheck.cpp

98

I originally had const *FuncDecl, but was told to use auto instead. I'll go ahead and make the change you suggest, but prefer to be explicit unless the type if hard to discern.

121

I'm probably missing something, but if I return FileMoveRange, i'm one token short, i.e., it doesn't cover the entire range I'm interested in.

However, if I extract begin and end, it works. I'll look into the difference, but if you have a better solution, please let me know.

clang-tidy/utils/LexerUtils.cpp

38

Not sure what you mean. Do you want me to change the name or add a comment to make that explicit?

[etienneb]

clang-tidy/modernize/UseNoexceptCheck.cpp

99

I'm not saying to use 'const *FuncDecl', but 'const auto*'

clang-tidy/utils/LexerUtils.cpp

38

If you call ParseTokens, it will parse tokens until it reach a semicolon/lbrace? exact?

[hintonda]

clang-tidy/modernize/UseNoexceptCheck.cpp

100

Sorry, I mis-typed. I meant to say I had, and prefer, "const FunctionDecl*" to "const auto *" in cases like this, but will do whatever you want.

clang-tidy/utils/LexerUtils.cpp

38

Yes, but again, I'm not sure what you are asking me to do/change.

Are you asking me to change something?

[hintonda]

Regenerated diff to make sure I pick up all changed files.

[hintonda]

clang-tidy/utils/LexerUtils.cpp

38

I regenerated the diff -- the last few arc diff commands left out a few files.

Anyway, as you can see, I lifted the ParseTokens() functions out of UseOverrideCheck.cpp and moved it here.

[aaron.ballman]

clang-tidy/modernize/UseNoexceptCheck.h

45–46

What is the difference between these two fields? The names are kind of confusing to me given their similarity, so perhaps renaming one to be more descriptive would be useful.

[hintonda]

clang-tidy/modernize/UseNoexceptCheck.h

45–46

The first is the option passed in, i.e., the replacement string we will use (either noexcept or a user macro like libcxx's _NOEXCEPT) for throw() and the second one is the string we'll actually use for a particular replacement, which can be what the user supplied, or noexcept(false) when the function can throw.

This was originally done with an auto in check(), but Alex wanted me to add a helper function to make check() easier to understand. So, I had to put it somewhere. I actually prefer the simpler original version.

I'll see if I can come up with better names, but suggestions are always welcome.

[alexfh]

clang-tidy/modernize/UseNoexceptCheck.cpp

38

Happy to rename it, but not sure making it more convenient serves much of a purpose.

Well, the initial problem was that the check method was too long and complicated. I suggested to make it easier to read by pulling a part of it in a separate method. I pointed to a part that seemed like something worth extracting to a method, and you mechanically created a method with unclear responsibilities, an awkward interface (two output parameters, a bool return value and a class data member) and a name that doesn't help understanding what the method does. That doesn't make the code any better. Let's try once again. We need to pull out an abstraction (at least one, but maybe you see more) that will make sense on its own and will have a reasonable interface. It could be SourceRange findDynamicExceptionSpecification(ArrayRef<Token> Tokens), for example.

122

I think, the problem is in the way you assign MoveRange in the checkHelper method. It is created as a character range and then you just assign its begin and end location. Instead, try MoveRange = CharSourceRange::getTokenRange(I->getLocation(), Loc).

clang-tidy/modernize/UseNoexceptCheck.h

45–46

The StringRef Replacement; should not be a member, since it's just one of the results of the checkHelper method.

[hintonda]

I agree -- things got messy.

I'm in the process of completely refactoring the whole thing -- including the ast matchers that I'll checkin later today. Thanks for your patience.

[hintonda]

• Complete refactor

[hintonda]

• Make helper functions static.

[hintonda]

• Added asserts for indexes into Tokens array.

[hintonda]

• A bit more refactoring and cleanup.

[hintonda]

I think this is now ready for review.

Thanks again for your patience...

[etienneb]

clang-tidy/modernize/UseNoexceptCheck.cpp

23

coding style:
MakeDynamicExceptionString -> makeDynamicExceptionString

35

MakeMoveRange -> makeMoveRange

(and all other occurrences below)

[hintonda]

• Fix some function names.

[hintonda]

• Revert back to previous version of the matcher and use Aaron's syntax with allOf and unless to achieve the same results -- much simpler.

[aaron.ballman]

clang-tidy/modernize/UseNoexceptCheck.cpp

22

Instead of a bunch of static functions, would an unnamed namespace make more sense?

35

Since this function is only called one time and is a one-liner, perhaps it makes more sense to inline the body at the call site?

47

Pathologically terrible counter-case: void func() throw(decltype(throw 12) *)

50

Can we use >= 0 instead of != -1? It makes it more immediately obvious that the array index will not underflow.

101

Please don't use auto here; I have no idea what type FileMoveRange will have from inspecting the code.

109

No need to quote %0, the diagnostics engine will already do it when passed a NamedDecl, so this will improperly double-quote the diagnostic.

clang-tidy/modernize/UseNoexceptCheck.h

23

I think this comment means noexcept(false) instead? Or is there a reason to replace with noexcept(true) instead of just noexcept?

test/clang-tidy/modernize-use-noexcept-macro.cpp

12

I may have missed some context in the discussion, but why shouldn't this trigger a FixItHint?

[hintonda]

clang-tidy/modernize/UseNoexceptCheck.cpp

22

Just following the pattern established in other checkers so as to minimize the number of changes, but will change to namespace.

35

Was a bit more involved, but last round of changes simplified it. Will simplify even more.

47

Good point, looks like I need a full fledged parser to catch 100% or the cases -- or we could ignore these corner cases.

clang-tidy/modernize/UseNoexceptCheck.h

23

Typo, should just be noexcept.

[aaron.ballman]

clang-tidy/modernize/UseNoexceptCheck.cpp

22

I don't have strong opinions; I think the usual rule of thumb boils down to whether the code fits on a page or not (if it does, use an unnamed namespace, if not, use static functions).

47

At the very least, we should have test cases showing what the behavior is with a big FIXME around this code, should you decide to keep it. I'm not keen on the idea of this being part of a fixit that may destroy well-defined user code. Same for the assumptions about the location of right parens. That code looks equally broken even without multiple throw tokens in the stream. Consider:
void func() throw(int(int));

[hintonda]

clang-tidy/modernize/UseNoexceptCheck.cpp

47

Had thought about adding paren parsing, but wasn't sure it was needed -- thanks for pointing out that it is. Of course, I'll have to parse from the beginning to do this correctly, but that's not a big deal.

50

Sure.

test/clang-tidy/modernize-use-noexcept-macro.cpp

12

Because the user provided a macro. The only reason you would do that is if you want the macro to expand to something different depending on whether or not 'noexcept' is supported.

Perhaps this can be done more elegantly, but the use case for this entire checker was libcxx. It defines _NOEXCEPT as either noexcept or throw() depending on whether or not noexcept is supported. I don't see a good way of doing that, other than removing it completely, so I just reported it without supplying a FixItHint.

[aaron.ballman]

test/clang-tidy/modernize-use-noexcept-macro.cpp

12

Ah, thank you for the context! That information should probably go in the user facing documentation somewhere.

[hintonda]

• Address additional comments.

[aaron.ballman]

clang-tidy/modernize/UseNoexceptCheck.cpp

40

@alexfh, what are your feelings on this FIXME? I am a bit concerned because these examples will cause the replacement range to be incorrect, which will turn working code into ill-formed code. The alternative, as I see it, is to instead properly track the exception specification source range information as part of the FunctionDecl (akin to FunctionDecl::getReturnTypeSourceRange()).

docs/clang-tidy/checks/modernize-use-noexcept.rst

41

This seems to run contrary to one of the examples below where a fixit is provided for `throw(A, B)`. If I understood properly, this statement is only true when using a ReplacementString. Perhaps it should instead say, "this check will detect, but not
provide a FixItHint for this case when a :option:ReplacementString is provided."

[hintonda]

clang-tidy/modernize/UseNoexceptCheck.cpp

40

Btw, I'm working on a fix I believe will handle all cases -- plan to checkin later today. However, it won't be that efficient unless I can find a way to match params that contain dynamic exception specifications. If they are only legal for function pointers -- which I think is the case -- that would make it easy and efficient, i.e., I wouldn't have to match all FunctionDecl's with one or more parameter and test them.

Is it possible to match a parameter that is a function pointer?

docs/clang-tidy/checks/modernize-use-noexcept.rst

41

This is only applicable when the user provided a replacement string other than noexcept. I'll try to make it clearer, however, there is a FIXME in the code concerning this. Specifically, should we allow replacement options for both noexcept and noexcept(false).

[alexfh]

clang-tidy/modernize/UseNoexceptCheck.cpp

23

It's much more common for LLVM code to use static functions:

http://llvm.org/docs/CodingStandards.html#anonymous-namespaces
"make anonymous namespaces as small as possible, and only use them for class declarations"

40

The alternative, as I see it, is to instead properly track the exception specification source range information as part of the FunctionDecl (akin to FunctionDecl::getReturnTypeSourceRange()).

It's all trade-offs: adding this information to the AST allows certain tasks in tooling to be done easier. On the other hand, this leads to bloating of the AST, which can already be huge. In this specific case, always keeping the source range of the exception specifier might be wasteful, since exception specifiers are rather rare (in my world, at least). But there might be a way to optionally store this information, e.g. in the ASTContext. In any case, makes sense to ask Richard Smith.

[aaron.ballman]

clang-tidy/modernize/UseNoexceptCheck.cpp

40

I am more curious as to your comfort level about committing a fixit that we know will break working code. ;-)

As for the suggested approach, I would obviously cover that with Richard, but I think we're going to need exception specifications more now that they're going to be part of the type system for C++. We have FunctionProtoTypeLoc for tracking this information.

[alexfh]

clang-tidy/modernize/UseNoexceptCheck.cpp

40

The important question here is how frequently is this likely to happen. I guess, it depends on the codebase, but maybe you have a rough guess?

[aaron.ballman]

clang-tidy/modernize/UseNoexceptCheck.cpp

40

Multiple uses of throw within a dynamic exception specification? Rare. Paren use that doesn't match what we expect? Infrequent, but likely to generate a bug report at some point depending on how often people use clang-tidy to check extensive code bases that actually *use* dynamic exception specifications. So my biggest concern really boils down to the fact that we're assuming the first right paren we come to after a "throw" keyword in a dynamic exception specification is the terminating right paren. I think we need to use paren matching instead. It would be nice if we could use BalancedDelimiterTracker to solve this, come to think of it.

[alexfh]

clang-tidy/modernize/UseNoexceptCheck.cpp

40

Yes, tracking balanced parenthesis sequences makes sense in any case.

[hintonda]

• First cut on a simple parser for decls.

Successfully parses all the examples I've been given so far. Please
help me break it.

[hintonda]

• Added another test.

[hintonda]

Improved matcher logic and add better range handling to try to deal
with multiple asserts concerning bad ranges when running checker again
real code, e.g., libcxx.

Even so, still seeing some asserts in Lexer::getSourceLocation(), e.g.:

Assertion failed: (Loc >= BufferStart && Loc <= BufferEnd &&
"Location out of range for this buffer!"), function
getSourceLocation, file
/Users/dhinton/projects/clang/llvm/tools/clang/lib/Lex/Lexer.cpp, l
ine 1073.

in a call to Sources.isBeforeInTranslationUnit(Range.getEnd(),
Tok.getLocation()) in parseDeclTokens().

Looks like my matcher is returning stuff that doesn't parse correctly.
Investigating.

[hintonda]

Adjust matcher, add better error checking, and additional test cases.

We can now parse all dynamic exception specifications, but haven't
been able to develop a matcher that will find the following function
declaration without finding other nodes that aren't parsable.

void g(void (*fp)(void) throw());

[hintonda]

Btw, this version can successfully check libcxx.

[alexfh]

Let's wait for http://reviews.llvm.org/D20428

[hintonda]

Fixed matcher -- added 'unless(isImplicit())'. Thanks to Aaron Ballman for the suggestion.

[hintonda]

In D18575#435388, @alexfh wrote:

Let's wait for http://reviews.llvm.org/D20428

No worries.

[hintonda]

Actually, this will never work correctly -- in fact, raw lexing will always be problematic. Consider:

void foo()
#if !__has_feature(cxx_noexcept)

throw(std::bad_alloc)

#endif
{}

In this case we *could* figure out if __has_feature(cxx_noexcept) evaluated to true or false, but what if it was a macro instead? We can't reliably handle it without reparsing the whole thing.

I'll leave it as is for now, but once http://reviews.llvm.org/D20428 is available, I'll start a new diff using it.

[hintonda]

Please see D20693 for an alternative approach.

[alexfh]

Cleaning up my Phab dashboard. If http://reviews.llvm.org/D20428 doesn't happen and we'll have to return to this implementation, please re-add me to the reviewers.

[hintonda]

This revision is OBE.