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