Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

Authored by hintonda on Mar 29 2016, 1:55 PM.



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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
hintonda updated this revision to Diff 57097.May 12 2016, 1:59 PM
  • Addressed a few comments for clarity/readability.
hintonda added inline comments.May 12 2016, 2:05 PM

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.


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.


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

etienneb added inline comments.May 12 2016, 2:16 PM

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


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

hintonda added inline comments.May 12 2016, 2:31 PM

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.


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

Are you asking me to change something?

hintonda updated this revision to Diff 57103.May 12 2016, 2:49 PM

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

hintonda added inline comments.May 12 2016, 2:53 PM

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 added inline comments.May 13 2016, 7:29 AM

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 added inline comments.May 13 2016, 8:02 AM

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 requested changes to this revision.May 14 2016, 5:32 PM
alexfh edited edge metadata.
alexfh added inline comments.

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.


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).


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

This revision now requires changes to proceed.May 14 2016, 5:32 PM

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 updated this revision to Diff 57298.May 15 2016, 9:38 AM
hintonda edited edge metadata.
  • Complete refactor
hintonda updated this revision to Diff 57299.May 15 2016, 10:15 AM
hintonda edited edge metadata.
  • Make helper functions static.
hintonda updated this revision to Diff 57301.May 15 2016, 11:16 AM
  • Added asserts for indexes into Tokens array.
hintonda updated this revision to Diff 57305.May 15 2016, 12:44 PM
  • A bit more refactoring and cleanup.

I think this is now ready for review.

Thanks again for your patience...

etienneb added inline comments.May 15 2016, 5:40 PM

coding style:
MakeDynamicExceptionString -> makeDynamicExceptionString


MakeMoveRange -> makeMoveRange

(and all other occurrences below)

hintonda updated this revision to Diff 57315.May 15 2016, 8:51 PM
  • Fix some function names.
hintonda updated this revision to Diff 57357.May 16 2016, 9:11 AM
  • 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 added inline comments.May 16 2016, 1:39 PM

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


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?


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


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


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


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


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


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

hintonda added inline comments.May 16 2016, 1:52 PM

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


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


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


Typo, should just be noexcept.

aaron.ballman added inline comments.May 16 2016, 2:14 PM

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).


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 added inline comments.May 16 2016, 2:26 PM

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.




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 added inline comments.May 16 2016, 2:29 PM

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

hintonda updated this revision to Diff 57432.May 16 2016, 9:31 PM
  • Address additional comments.
aaron.ballman added inline comments.May 17 2016, 6:11 AM

@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()).


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 added inline comments.May 17 2016, 10:47 AM

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?


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 added inline comments.May 17 2016, 2:35 PM

It's much more common for LLVM code to use static functions:
"make anonymous namespaces as small as possible, and only use them for class declarations"


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 added inline comments.May 17 2016, 2:41 PM

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 added inline comments.May 17 2016, 3:24 PM

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 added inline comments.May 17 2016, 3:45 PM

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 added inline comments.May 17 2016, 3:54 PM

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

hintonda updated this revision to Diff 57547.May 17 2016, 6:08 PM
  • 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 updated this revision to Diff 57555.May 17 2016, 7:42 PM
  • Added another test.
hintonda updated this revision to Diff 57686.May 18 2016, 3:25 PM

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.

hintonda updated this revision to Diff 57811.May 19 2016, 9:17 AM

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());

Btw, this version can successfully check libcxx.

alexfh requested changes to this revision.May 20 2016, 2:40 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.May 20 2016, 2:40 AM
hintonda updated this revision to Diff 58151.May 23 2016, 2:19 PM
hintonda edited edge metadata.

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

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

void foo()
#if !__has_feature(cxx_noexcept)



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 is available, I'll start a new diff using it.

Please see D20693 for an alternative approach.

alexfh resigned from this revision.Jun 3 2016, 3:13 PM
alexfh removed a reviewer: alexfh.

Cleaning up my Phab dashboard. If doesn't happen and we'll have to return to this implementation, please re-add me to the reviewers.

hintonda abandoned this revision.Jun 3 2016, 3:17 PM

This revision is OBE.