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 <mailto: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 <mailto: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 <mailto: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

Reply via email to