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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:39
@@ +38,3 @@
+
+  // FIXME: Add paren matching so we can parse more complex throw statements,
+  // e.g., (examples provided by Aaron Ballman):
----------------
aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > hintonda wrote:
> > > > > aaron.ballman wrote:
> > > > > > @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()`).
> > > > > 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?
> > > > > 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.
> > > 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.
> > 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?
> 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.
Yes, tracking balanced parenthesis sequences makes sense in any case.


http://reviews.llvm.org/D18575