*nod* Maybe consistency's enough for now. But yeah, if you can test whether the assertion fires (though for invalid locs, that usually means invalid code - and we don't have nice big repositories of weird invalid code - just the clang regression tests).
On Tue, Aug 7, 2018 at 3:27 PM Stephen Kelly <steve...@gmail.com> wrote: > > Ok, I can look into adding the assertion and run the tests with it to see > if anything comes up. > > I made a tool which dumps SourceLocations reachable from an AST node (I > intend to integrate it into clang-query), and I noticed the large amount of > mixing of get{Start,End}Loc and getLoc{Start,End} (see other pending > reviews). I reviewed all of them and found that this method is the only > case where a pair of methods of that name pattern differ in what they > return (by eye, at least), so I thought it should be fixed. > > Thanks, > > Stephen. > > > On 07/08/18 23:18, David Blaikie wrote: > > If it never comes up, maybe an assertion would suffice? (& if the > assertion ever does fire - hey, we've found a test case to use) > > How'd you find this/what motivated you to make the change? > > On Tue, Aug 7, 2018 at 3:11 PM Stephen Kelly <steve...@gmail.com> wrote: > >> >> Hi David, >> >> I'm happy to add a test case, but I don't know how to catch this case. >> It's not obvious to me if any code path intentionally creates a >> DeclarationNameInfo with a valid start loc and an invalid end loc. Can you >> suggest a test case? >> >> Thanks, >> >> Stephen. >> >> >> On 07/08/18 03:23, David Blaikie wrote: >> >> test case? >> >> On Mon, Jul 30, 2018 at 1:39 PM Stephen Kelly via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: steveire >>> Date: Mon Jul 30 13:39:14 2018 >>> New Revision: 338301 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=338301&view=rev >>> Log: >>> Avoid returning an invalid end source loc >>> >>> Modified: >>> cfe/trunk/include/clang/AST/DeclarationName.h >>> cfe/trunk/lib/AST/DeclarationName.cpp >>> >>> Modified: cfe/trunk/include/clang/AST/DeclarationName.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclarationName.h?rev=338301&r1=338300&r2=338301&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/AST/DeclarationName.h (original) >>> +++ cfe/trunk/include/clang/AST/DeclarationName.h Mon Jul 30 13:39:14 >>> 2018 >>> @@ -558,7 +558,7 @@ public: >>> SourceLocation getBeginLoc() const { return NameLoc; } >>> >>> /// getEndLoc - Retrieve the location of the last token. >>> - SourceLocation getEndLoc() const; >>> + SourceLocation getEndLoc() const { return getLocEnd(); } >>> >>> /// getSourceRange - The range of the declaration name. >>> SourceRange getSourceRange() const LLVM_READONLY { >>> @@ -570,9 +570,11 @@ public: >>> } >>> >>> SourceLocation getLocEnd() const LLVM_READONLY { >>> - SourceLocation EndLoc = getEndLoc(); >>> + SourceLocation EndLoc = getEndLocPrivate(); >>> return EndLoc.isValid() ? EndLoc : getLocStart(); >>> } >>> +private: >>> + SourceLocation getEndLocPrivate() const; >>> }; >>> >>> /// Insertion operator for diagnostics. This allows sending >>> DeclarationName's >>> >>> Modified: cfe/trunk/lib/AST/DeclarationName.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclarationName.cpp?rev=338301&r1=338300&r2=338301&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/AST/DeclarationName.cpp (original) >>> +++ cfe/trunk/lib/AST/DeclarationName.cpp Mon Jul 30 13:39:14 2018 >>> @@ -689,7 +689,7 @@ void DeclarationNameInfo::printName(raw_ >>> llvm_unreachable("Unexpected declaration name kind"); >>> } >>> >>> -SourceLocation DeclarationNameInfo::getEndLoc() const { >>> +SourceLocation DeclarationNameInfo::getEndLocPrivate() const { >>> switch (Name.getNameKind()) { >>> case DeclarationName::Identifier: >>> case DeclarationName::CXXDeductionGuideName: >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits