Location and Range unittests for FriendDecl

[nikola]

Summary

Any other interesting scenarios to test while am at it?

[klimek]

This looks like a positive step, so LG.
(I assume you have checked that at least of them fails without your patch ;)

Richard might have more ideas for corner cases (he's a master of corner cases)

[klimek]

+richard for corner case generation

[nikola]

Yep, the one with friend class fails without my patch.

[nikola]

Committed these tests in r210306 but I'll leave this review request open in case the following code produces something in the next week or so :)

richard_smith_engine e(beer()); // not sure if this is a proper way to seed the engine
obscure_frienddecl_distribution d(e);
d();

[nikola]

Turns out we didn't have correct location and range for class template friend. The same was true for case where friend is not the first token as in Richard's inline example.

Manuel:
Are my constructor/destructor matchers good?

Richard:

1. What's the TypeSourceInfo case in FriendDecl::getSourceRange?
2. I couldn't find a way to hit the first call to FriendDecl::Create inside Sema::ActOnTemplatedFriendTag (some kind of explicit specialization case). I tried issuing a random diagnostic from there hoping it would break something in the test suite but it seems we don't have any coverage for this?

[rsmith]

One comment, the rest looks fine.

include/clang/AST/DeclFriend.h

148

This use of < is not correct (it'll do the wrong thing in the presence of macro expansion). SourceManager::isBeforeInTranslationUnit is the right way, but it's hard to get at from here. What does a friend function declaration think its LocStart is? Does that include the friend token already? Maybe you can just use that.

[nikola]

include/clang/AST/DeclFriend.h

148

This is very unfortunate but getStartLoc depends on the range, it's whatever beginning of the range is :(

Note that we need this only for that contrived case of yours ;) where friend isn't the first token in the declaration. What's worse, having wrong range in that case or in the presence of macro expansion? Who uses macros anyway :P

I tried this but friend keyword and start of NamedDecl point to same location. Could we already be wrong in this case?

#define DECL void f();
struct A{
  friend DECL
};

[nikola]

Ping.

[nikola]

This covers all test cases.

[nikola]

Committed in r213220.

include/clang/AST/DeclFriend.h

     if (NamedDecl *ND = getFriendDecl()) {
       if (FunctionTemplateDecl *FTD = dyn_cast<FunctionTemplateDecl>(ND))
         return FTD->getSourceRange();
+      if (ClassTemplateDecl *CTD = dyn_cast<ClassTemplateDecl>(ND))
+        return CTD->getSourceRange();
       if (DeclaratorDecl *DD = dyn_cast<DeclaratorDecl>(ND)) {
         if (DD->getOuterLocStart() != DD->getInnerLocStart())
           return DD->getSourceRange();
       }
-      return SourceRange(getFriendLoc(), ND->getLocEnd());
+      SourceLocation FriendLoc = getFriendLoc();
+      SourceLocation DeclLoc = ND->getLocStart();
+      return SourceRange(FriendLoc < DeclLoc ? FriendLoc : DeclLoc, 
+                         ND->getLocEnd());
     }
     else if (TypeSourceInfo *TInfo = getFriendType()) {
       SourceLocation StartL = (NumTPLists == 0)

include/clang/Sema/Sema.h

                                 TemplateParameterList *TemplateParams,
                                 AccessSpecifier AS,
                                 SourceLocation ModulePrivateLoc,
+                                SourceLocation FriendLoc,
                                 unsigned NumOuterTemplateParamLists,
                             TemplateParameterList **OuterTemplateParamLists);
 

lib/Sema/SemaDecl.cpp

                                                SS, Name, NameLoc, Attr,
                                                TemplateParams, AS,
                                                ModulePrivateLoc,
+                                               /*FriendLoc*/SourceLocation(),
                                                TemplateParameterLists.size()-1,
                                                TemplateParameterLists.data());
         return Result.get();

lib/Sema/SemaDeclCXX.cpp

       if (Invalid)
         return nullptr;
 
-      return CheckClassTemplate(S, TagSpec, TUK_Friend, TagLoc,
-                                SS, Name, NameLoc, Attr,
-                                TemplateParams, AS_public,
+      return CheckClassTemplate(S, TagSpec, TUK_Friend, TagLoc, SS, Name,
+                                NameLoc, Attr, TemplateParams, AS_public,
                                 /*ModulePrivateLoc=*/SourceLocation(),
-                                TempParamLists.size() - 1,
+                                FriendLoc, TempParamLists.size() - 1,
                                 TempParamLists.data()).get();
     } else {
       // The "template<>" header is extraneous.

lib/Sema/SemaTemplate.cpp

                          AttributeList *Attr,
                          TemplateParameterList *TemplateParams,
                          AccessSpecifier AS, SourceLocation ModulePrivateLoc,
+                         SourceLocation FriendLoc,
                          unsigned NumOuterTemplateParamLists,
                          TemplateParameterList** OuterTemplateParamLists) {
   assert(TemplateParams && TemplateParams->size() > 0 &&

                           /* AddToContext = */ false);
     }
 
-    FriendDecl *Friend = FriendDecl::Create(Context, CurContext,
-                                            NewClass->getLocation(),
-                                            NewTemplate,
-                                    /*FIXME:*/NewClass->getLocation());
+    FriendDecl *Friend = FriendDecl::Create(
+        Context, CurContext, NewClass->getLocation(), NewTemplate, FriendLoc);
     Friend->setAccess(AS_public);
     CurContext->addDecl(Friend);
   }

                                 Attr,
                                 TemplateParams,
                                 AS_none, /*ModulePrivateLoc=*/SourceLocation(),
+                                /*FriendLoc*/SourceLocation(),
                                 TemplateParameterLists.size() - 1,
                                 TemplateParameterLists.data());
     }

unittests/AST/SourceLocationTest.cpp

       unresolvedUsingValueDecl()));
 }
 
-TEST(FriendDecl, FriendFunctionLocation) {
+TEST(FriendDecl, FriendNonMemberFunctionLocation) {
   LocationVerifier<FriendDecl> Verifier;
   Verifier.expectLocation(2, 13);
   EXPECT_TRUE(Verifier.match("struct A {\n"

                              friendDecl()));
 }
 
-TEST(FriendDecl, FriendFunctionRange) {
+TEST(FriendDecl, FriendNonMemberFunctionRange) {
   RangeVerifier<FriendDecl> Verifier;
   Verifier.expectRange(2, 1, 2, 15);
   EXPECT_TRUE(Verifier.match("struct A {\n"

                              friendDecl()));
 }
 
-TEST(FriendDecl, FriendClassLocation) {
+TEST(FriendDecl, FriendNonMemberFunctionDefinitionLocation) {
+  LocationVerifier<FriendDecl> Verifier;
+  Verifier.expectLocation(2, 12);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "friend int f() { return 0; }\n"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendNonMemberFunctionDefinitionRange) {
+  RangeVerifier<FriendDecl> Verifier;
+  Verifier.expectRange(2, 1, 2, 28);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "friend int f() { return 0; }\n"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendElaboratedTypeLocation) {
   LocationVerifier<FriendDecl> Verifier;
   Verifier.expectLocation(2, 8);
   EXPECT_TRUE(Verifier.match("struct A {\n"

                              friendDecl()));
 }
 
-TEST(FriendDecl, FriendClassRange) {
+TEST(FriendDecl, FriendElaboratedTypeRange) {
   RangeVerifier<FriendDecl> Verifier;
   Verifier.expectRange(2, 1, 2, 14);
   EXPECT_TRUE(Verifier.match("struct A {\n"

                              friendDecl()));
 }
 
+TEST(FriendDecl, FriendSimpleTypeLocation) {
+  LocationVerifier<FriendDecl> Verifier;
+  Verifier.expectLocation(3, 8);
+  EXPECT_TRUE(Verifier.match("class B;\n"
+                             "struct A {\n"
+                             "friend B;\n"
+                             "};\n",
+                             friendDecl(), Lang_CXX11));
+}
+
+TEST(FriendDecl, FriendSimpleTypeRange) {
+  RangeVerifier<FriendDecl> Verifier;
+  Verifier.expectRange(3, 1, 3, 8);
+  EXPECT_TRUE(Verifier.match("class B;\n"
+                             "struct A {\n"
+                             "friend B;\n"
+                             "};\n",
+                             friendDecl(), Lang_CXX11));
+}
+
 TEST(FriendDecl, FriendTemplateParameterLocation) {
   LocationVerifier<FriendDecl> Verifier;
   Verifier.expectLocation(3, 8);

                              friendDecl(), Lang_CXX11));
 }
 
+TEST(FriendDecl, FriendConstructorDestructorLocation) {
+  const std::string Code = "struct B {\n"
+                           "B();\n"
+                           "~B();\n"
+                           "};\n"
+                           "struct A {\n"
+                           "friend B::B(), B::~B();\n"
+                           "};\n";
+  LocationVerifier<FriendDecl> ConstructorVerifier;
+  ConstructorVerifier.expectLocation(6, 11);
+  EXPECT_TRUE(ConstructorVerifier.match(
+      Code, friendDecl(has(constructorDecl(ofClass(hasName("B")))))));
+  LocationVerifier<FriendDecl> DestructorVerifier;
+  DestructorVerifier.expectLocation(6, 19);
+  EXPECT_TRUE(DestructorVerifier.match(
+      Code, friendDecl(has(destructorDecl(ofClass(hasName("B")))))));
+}
+
+TEST(FriendDecl, FriendConstructorDestructorRange) {
+  const std::string Code = "struct B {\n"
+                           "B();\n"
+                           "~B();\n"
+                           "};\n"
+                           "struct A {\n"
+                           "friend B::B(), B::~B();\n"
+                           "};\n";
+  RangeVerifier<FriendDecl> ConstructorVerifier;
+  ConstructorVerifier.expectRange(6, 1, 6, 13);
+  EXPECT_TRUE(ConstructorVerifier.match(
+      Code, friendDecl(has(constructorDecl(ofClass(hasName("B")))))));
+  RangeVerifier<FriendDecl> DestructorVerifier;
+  DestructorVerifier.expectRange(6, 1, 6, 22);
+  EXPECT_TRUE(DestructorVerifier.match(
+      Code, friendDecl(has(destructorDecl(ofClass(hasName("B")))))));
+}
+
+TEST(FriendDecl, FriendTemplateFunctionLocation) {
+  LocationVerifier<FriendDecl> Verifier;
+  Verifier.expectLocation(3, 13);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "template <typename T>\n"
+                             "friend void f();\n"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendTemplateFunctionRange) {
+  RangeVerifier<FriendDecl> Verifier;
+  Verifier.expectRange(2, 1, 3, 15);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "template <typename T>\n"
+                             "friend void f();\n"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendTemplateClassLocation) {
+  LocationVerifier<FriendDecl> Verifier;
+  Verifier.expectLocation(3, 14);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "template <typename T>\n"
+                             "friend class B;\n"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendTemplateClassRange) {
+  RangeVerifier<FriendDecl> Verifier;
+  Verifier.expectRange(2, 1, 3, 14);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "template <typename T>\n"
+                             "friend class B;\n"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendInlineFunctionLocation) {
+  LocationVerifier<FriendDecl> Verifier;
+  Verifier.expectLocation(2, 19);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "int inline friend f() { return 0; }"
+                             "};\n",
+                             friendDecl()));
+}
+
+TEST(FriendDecl, FriendInlineFunctionRange) {
+  RangeVerifier<FriendDecl> Verifier;
+  Verifier.expectRange(2, 1, 2, 35);
+  EXPECT_TRUE(Verifier.match("struct A {\n"
+                             "int inline friend f() { return 0; }"
+                             "};\n",
+                             friendDecl(), Lang_CXX11));
+}
+
 TEST(FriendDecl, InstantiationSourceRange) {
   RangeVerifier<FriendDecl> Verifier;
   Verifier.expectRange(4, 3, 4, 35);