Re: [PATCH] D20428: Tracking exception specification source locations

On Fri, Jun 24, 2016 at 8:39 AM, Richard Smith wrote: > rsmith added a comment. > > Ah right, we were (intentionally, but unfortunately) making an assumption > that the `TypeLoc` data layout doesn't change when the exception spec of a > function is updated. You'd need to

Re: [PATCH] D20428: Tracking exception specification source locations

On Fri, Jun 24, 2016 at 8:39 AM, Richard Smith wrote: > rsmith added a comment. > > Ah right, we were (intentionally, but unfortunately) making an assumption > that the `TypeLoc` data layout doesn't change when the exception spec of a > function is updated. You'd need to

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added a comment. In http://reviews.llvm.org/D20428#452680, @hintonda wrote: > The comment says to rebuild TypeSourceInfo, but isn't that what this does? > > if (TSInfo->getType() != FD->getType()) > Updated = getFunctionTypeWithExceptionSpec(*this, TSInfo->getType(), ESI); >

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added a comment. Richard, with the following test case, my patch currently fails an assertion in `ASTContext::adjustExceptionSpec()` that I want to solve before committing: template void f7() { struct S { void g() noexcept(undefined_val); }; // expected-error{{use of

Re: [PATCH] D20428: Tracking exception specification source locations

rsmith accepted this revision. This revision is now accepted and ready to land. Comment at: lib/AST/Decl.cpp:2938-2948 @@ -2937,1 +2937,13 @@ +SourceRange FunctionDecl::getExceptionSpecSourceRange() const { + const TypeSourceInfo *TSI = getTypeSourceInfo(); + if (!TSI) +

Re: [PATCH] D20428: Tracking exception specification source locations

hintonda added a comment. The comment says to rebuild TypeSourceInfo, but isn't that what this does? if (TSInfo->getType() != FD->getType()) Updated = getFunctionTypeWithExceptionSpec(*this, TSInfo->getType(), ESI); TSInfo->overrideType(Updated); If so, could you fix this by either

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added a comment. One thing this patch does not do but needs to is fix `ASTContext::adjustExceptionSpec()` (Thanks to Don Hinton for pointing this out off-list!), however, I am at a bit of a loss for how best to rebuild the type location. Would it be correct to call

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added a comment. @rsmith: Ping http://reviews.llvm.org/D20428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20428: Tracking exception specification source locations

hintonda added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:3403-3428 @@ -3402,6 +3402,7 @@ // If we already had a dynamic specification, parse the noexcept for, // recovery, but emit a diagnostic and don't store the results. - SourceRange NoexceptRange; +

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman updated this revision to Diff 58804. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updating based on further review comments. http://reviews.llvm.org/D20428 Files: include/clang/AST/Decl.h include/clang/AST/TypeLoc.h lib/AST/Decl.cpp

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:3403-3428 @@ -3402,6 +3402,7 @@ // If we already had a dynamic specification, parse the noexcept for, // recovery, but emit a diagnostic and don't store the results. - SourceRange NoexceptRange; +

Re: [PATCH] D20428: Tracking exception specification source locations

hintonda added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:3403-3428 @@ -3402,6 +3402,7 @@ // If we already had a dynamic specification, parse the noexcept for, // recovery, but emit a diagnostic and don't store the results. - SourceRange NoexceptRange; +

Re: [PATCH] D20428: Tracking exception specification source locations

hintonda added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:3403-3428 @@ -3402,6 +3402,7 @@ // If we already had a dynamic specification, parse the noexcept for, // recovery, but emit a diagnostic and don't store the results. - SourceRange NoexceptRange; +

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman updated this revision to Diff 58784. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Updated based on review comments. http://reviews.llvm.org/D20428 Files: include/clang/AST/Decl.h include/clang/AST/TypeLoc.h lib/AST/Decl.cpp

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:3403-3427 @@ -3402,5 +3402,6 @@ // recovery, but emit a diagnostic and don't store the results. - SourceRange NoexceptRange; + SourceRange NoexceptRange(Tok.getLocation(), +

Re: [PATCH] D20428: Tracking exception specification source locations

rsmith added inline comments. Comment at: include/clang/AST/TypeLoc.h:1251 @@ -1250,2 +1250,3 @@ SourceLocation RParenLoc; + SourceRange ExceptionSpecRange; SourceLocation LocalRangeEnd; Can you arrange things so we only store this if there is an exception

Re: [PATCH] D20428: Tracking exception specification source locations

rsmith added a comment. Seems reasonable to me. Comment at: lib/Parse/ParseDeclCXX.cpp:3403-3427 @@ -3402,5 +3402,6 @@ // recovery, but emit a diagnostic and don't store the results. - SourceRange NoexceptRange; + SourceRange NoexceptRange(Tok.getLocation(), +

Re: [PATCH] D20428: Tracking exception specification source locations

alexfh added a comment. Richard, ping. http://reviews.llvm.org/D20428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20428: Tracking exception specification source locations

hintonda added a comment. Sure that sounds good to me. However, I would like to learn how to write better ASTMatchers. In any case, this has been a great learning experience. http://reviews.llvm.org/D20428 ___ cfe-commits mailing list

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added a comment. In http://reviews.llvm.org/D20428#434420, @hintonda wrote: > I can already catch all of these cases, but I can't catch this one, will this > catch it too? > > void g(void (*fp)(void) throw()) throw(); > ^^^ > > > Actually, I can

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman updated this revision to Diff 57816. aaron.ballman added a comment. Added test cases for parameter types that have exception specifications. http://reviews.llvm.org/D20428 Files: include/clang/AST/Decl.h include/clang/AST/TypeLoc.h lib/AST/Decl.cpp

Re: [PATCH] D20428: Tracking exception specification source locations

hintonda added a comment. I can already catch all of these cases, but I can't catch this one, will this catch it too? void g(void (*fp)(void) throw()) throw(); ^^^ http://reviews.llvm.org/D20428 ___ cfe-commits

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman added a comment. In http://reviews.llvm.org/D20428#434270, @alexfh wrote: > In http://reviews.llvm.org/D20428#434242, @aaron.ballman wrote: > > > In http://reviews.llvm.org/D20428#434238, @alexfh wrote: > > > > > Full context diff, please. > > > > > > Pardon my complete ignorance,

Re: [PATCH] D20428: Tracking exception specification source locations

aaron.ballman updated this revision to Diff 57798. aaron.ballman added a comment. For noexcept specifications without an expression, now tracking the full source range. Also, added tests. http://reviews.llvm.org/D20428 Files: include/clang/AST/Decl.h include/clang/AST/TypeLoc.h

Re: [PATCH] D20428: Tracking exception specification source locations

alexfh added a comment. In http://reviews.llvm.org/D20428#434242, @aaron.ballman wrote: > In http://reviews.llvm.org/D20428#434238, @alexfh wrote: > > > Full context diff, please. > > > Pardon my complete ignorance, but how? I generated the diff from svn the > usual way, so I assume I've missed

Re: [PATCH] D20428: Tracking exception specification source locations

alexfh added a comment. Full context diff, please. > I'm not certain of the best way to test this functionality in isolation; Same way other locations/ranges are tested in unittests/AST/SourceLocationTest.cpp? http://reviews.llvm.org/D20428 ___