[libclang] When getting platform availabilities, merge multiple declarations if possible

[arphaman]

tools/libclang/CIndex.cpp

7253

I think that you can move this if before the new if, and convert the new if to be an if (AvailabilityAttrs.empty()) that returns early. Then you can do the merging outside of the if at the end of the function.

7337

You can use a ranged for loop here if you use take_front, e.g.

for (const auto *Avail : AvailabilityAttrs.take_front(availability_size))

[rdwampler]

tools/libclang/CIndex.cpp

7337

I would need to covert this to an ArrayRef, I believe. Also, I would need to check availability_size is in bounds. Would something like the following be preferred:

int N = 0;
for (const auto *Avail : AvailabilityAttrs) {
  if (N < availability_size) {
    // populate availability
    N++;
  }
}

[arphaman]

tools/libclang/CIndex.cpp

7337

Right, sorry I forgot about the N.
I think that you can use llvm::enumerate(llvm::makeArrayRef(AvailabilityAttrs).take_front(availability_size)) to get rid of N as well, as you can the index and the attribute pointer from the loop variable.
Btw, The take_front(N) will ensure that you will only iterate either over the first N attributes or all of the attributes if N > AvailabilityAttrs.size(), so you won't need to check N in the loop.

[rdwampler]

Rearrange if statements in getCursorPlatformAvailabilityForDecl to return early if we do not need to merge availability attributes.
Use ranged for loop.

[arphaman]

It seems that there's a slight bug in the patch:

If I print the output of the following code using c-index-test -test-load-source all:

void bar2(void)
__attribute__((availability(macosx, introduced=10.4)))
__attribute__((availability(macosx, deprecated=10.5, message="use x")));

I see availability that includes a deprecation message: FunctionDecl=bar2:15:6 (deprecated) (macos, introduced=10.4, deprecated=10.5, message="use x").
However, if I look at the output of the following code:

void bar2(void)
__attribute__((availability(macosx, deprecated=10.5, message="use x")))
__attribute__((availability(macosx, introduced=10.4)));

I get something that doesn't include the message: FunctionDecl=bar2:15:6 (deprecated) (macos, introduced=10.4, deprecated=10.5).

test/Index/availability.c

13

Please add a test for merge of the obsoleted clause as well.

tools/libclang/CIndex.cpp

7256–7307

You should be able to keep the return here, since you will sort and unique the attributes in this call to getCursorPlatformAvailabilityForDecl.

7272

Please invert this if and return false early instead of leaving it at the end of the lambda.

[rdwampler]

This should resolve the bug (a conditional was inverted) in the last revision along with the other changes requested.

[arphaman]

This looks better, it's almost ready. A couple of small requests:

tools/libclang/CIndex.cpp

7278

We should also have a test that verifies that we merge identical availabilities.

7284

I think that we don't really need the (!LHS->getMessage().empty() && !RHS->getMessage().empty()) check here since message has to be either in a deprecated or obsoleted clause, so we should already handle that with previous checks.

[rdwampler]

test/Index/availability.c

14

Can we run FileCheck once now? I believe the CHECK-1 and CHECK-2 were added since there were no particular order for the availabilities. With this patch the order is guarantee to be stable.

tools/libclang/CIndex.cpp

7284

Agreed.

[arphaman]

test/Index/availability.c

14

Sure, if the test passes we can use one FileCheck invocation. We can also use CHECK-DAG: to check for strings without obeying a specific order.

[rdwampler]

Remove an unnecessary check when determining a mismatch of availabilities
Simplify invocation of FileCheck in availability.c

[arphaman]

LGTM

[arphaman]

Do you have commit access or shall I commit it on your behalf?

[rdwampler]

I don't have commit access, but I would grateful if you can commit it.

[arphaman]

Ronald, I had to revert the commit as the availability.c test was failing on Linux (e.g. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/12532). Can you please look into that? I think you might have to got back to the CHECK-1 and CHECK-2 thing

[rdwampler]

Alex, I will look into it. Thanks!

[rdwampler]

I was able to build and test this on a linux box. The issue was the whitespace surrounding the regex.

On Linux, (unavailable) is not present. I.e,
FunctionDecl=foo:3:6 (ios, introduced=3.2, deprecated=4.1) (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)
vs on macOS
FunctionDecl=foo:3:6 (unavailable) (ios, introduced=3.2, deprecated=4.1) (unavailable) (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)

I changed it to just match any characters between the function declaration and the availabilities.

Sorry for the noise.

[arphaman]

Thanks, I'll recommit today.

[arphaman]

Recommitted in r305221.

test/Index/availability.c

 // Run lines below; this test is line- and column-sensitive.
 
 void foo(void) __attribute__((availability(macosx,introduced=10.4,deprecated=10.5,obsoleted=10.7), availability(ios,introduced=3.2,deprecated=4.1)));
 
 enum {
   old_enum
 } __attribute__((deprecated));
 
 enum {
   old_enum_plat
-} __attribute__((availability(macosx,introduced=10.4,deprecated=10.5,obsoleted=10.7)
+} __attribute__((availability(macosx,introduced=10.4,deprecated=10.5,obsoleted=10.7)));
 
-// RUN: c-index-test -test-load-source all %s > %t
-// RUN: FileCheck -check-prefix=CHECK-1 %s < %t
-// RUN: FileCheck -check-prefix=CHECK-2 %s < %t
-// CHECK-1: (ios, introduced=3.2, deprecated=4.1) 
-// CHECK-2: (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)
+void bar(void) __attribute__((availability(macosx,introduced=10.4))) __attribute__((availability(macosx,obsoleted=10.6))) __attribute__((availability(ios,introduced=3.2))) __attribute__((availability(macosx,deprecated=10.5,message="use foobar")));
 
-// CHECK-2: EnumConstantDecl=old_enum:6:3 (Definition) (deprecated)
-// CHECK-2: EnumConstantDecl=old_enum_plat:10:3 {{.*}} (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)
+void bar2(void) __attribute__((availability(macosx,introduced=10.4,deprecated=10.5,obsoleted=10.7))) __attribute__((availability(ios,introduced=3.2,deprecated=10.0))) __attribute__((availability(macosx,introduced=10.4,deprecated=10.5,obsoleted=10.7))) __attribute__((availability(ios,introduced=3.2,deprecated=10.0)));
+
+// RUN: c-index-test -test-load-source all %s | FileCheck %s
+// CHECK: FunctionDecl=foo:3:6{{.*}}(ios, introduced=3.2, deprecated=4.1) (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)
+// CHECK: EnumConstantDecl=old_enum:6:3 (Definition) (deprecated)
+// CHECK: EnumConstantDecl=old_enum_plat:10:3 {{.*}} (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)
+// CHECK: FunctionDecl=bar:13:6{{.*}}(ios, introduced=3.2) (macos, introduced=10.4, deprecated=10.5, obsoleted=10.6, message="use foobar")
+// CHECK: FunctionDecl=bar2:15:6{{.*}}(ios, introduced=3.2, deprecated=10.0) (macos, introduced=10.4, deprecated=10.5, obsoleted=10.7)

tools/libclang/CIndex.cpp

static CXVersion convertVersion(VersionTuple In) {
 
   Optional<unsigned> Subminor = In.getSubminor();
   if (Subminor.hasValue())
     Out.Subminor = *Subminor;
   
   return Out;
 }
 
-static int getCursorPlatformAvailabilityForDecl(const Decl *D,
-                                                int *always_deprecated,
-                                                CXString *deprecated_message,
-                                                int *always_unavailable,
-                                                CXString *unavailable_message,
-                                           CXPlatformAvailability *availability,
-                                                int availability_size) {
+static void getCursorPlatformAvailabilityForDecl(
+    const Decl *D, int *always_deprecated, CXString *deprecated_message,
+    int *always_unavailable, CXString *unavailable_message,
+    SmallVectorImpl<AvailabilityAttr *> &AvailabilityAttrs) {
   bool HadAvailAttr = false;
-  int N = 0;
   for (auto A : D->attrs()) {
     if (DeprecatedAttr *Deprecated = dyn_cast<DeprecatedAttr>(A)) {
       HadAvailAttr = true;
       if (always_deprecated)
         *always_deprecated = 1;
       if (deprecated_message) {
         clang_disposeString(*deprecated_message);
         *deprecated_message = cxstring::createDup(Deprecated->getMessage());
       }
       continue;
     }
-    
     if (UnavailableAttr *Unavailable = dyn_cast<UnavailableAttr>(A)) {
       HadAvailAttr = true;
       if (always_unavailable)
         *always_unavailable = 1;
       if (unavailable_message) {
         clang_disposeString(*unavailable_message);
         *unavailable_message = cxstring::createDup(Unavailable->getMessage());
       }
       continue;
     }
-    
     if (AvailabilityAttr *Avail = dyn_cast<AvailabilityAttr>(A)) {
+      AvailabilityAttrs.push_back(Avail);
       HadAvailAttr = true;
-      if (N < availability_size) {
-        availability[N].Platform
-          = cxstring::createDup(Avail->getPlatform()->getName());
-        availability[N].Introduced = convertVersion(Avail->getIntroduced());
-        availability[N].Deprecated = convertVersion(Avail->getDeprecated());
-        availability[N].Obsoleted = convertVersion(Avail->getObsoleted());
-        availability[N].Unavailable = Avail->getUnavailable();
-        availability[N].Message = cxstring::createDup(Avail->getMessage());
-      }
-      ++N;
     }
   }
 
   if (!HadAvailAttr)
     if (const EnumConstantDecl *EnumConst = dyn_cast<EnumConstantDecl>(D))
       return getCursorPlatformAvailabilityForDecl(
-                                        cast<Decl>(EnumConst->getDeclContext()),
-                                                  always_deprecated,
-                                                  deprecated_message,
-                                                  always_unavailable,
-                                                  unavailable_message,
-                                                  availability,
-                                                  availability_size);
-  
-  return N;
+          cast<Decl>(EnumConst->getDeclContext()), always_deprecated,
+          deprecated_message, always_unavailable, unavailable_message,
+          AvailabilityAttrs);
+  if (AvailabilityAttrs.empty())
+    return;
+
+  std::sort(AvailabilityAttrs.begin(), AvailabilityAttrs.end(),
+            [](AvailabilityAttr *LHS, AvailabilityAttr *RHS) {
+              return LHS->getPlatform() > RHS->getPlatform();
+            });
+  ASTContext &Ctx = D->getASTContext();
+  auto It = std::unique(
+      AvailabilityAttrs.begin(), AvailabilityAttrs.end(),
+      [&Ctx](AvailabilityAttr *LHS, AvailabilityAttr *RHS) {
+        if (LHS->getPlatform() != RHS->getPlatform())
+          return false;
+
+        if (LHS->getIntroduced() == RHS->getIntroduced() &&
+            LHS->getDeprecated() == RHS->getDeprecated() &&
+            LHS->getObsoleted() == RHS->getObsoleted() &&
+            LHS->getMessage() == RHS->getMessage() &&
+            LHS->getReplacement() == RHS->getReplacement())
+          return true;
+
+        if ((!LHS->getIntroduced().empty() && !RHS->getIntroduced().empty()) ||
+            (!LHS->getDeprecated().empty() && !RHS->getDeprecated().empty()) ||
+            (!LHS->getObsoleted().empty() && !RHS->getObsoleted().empty()))
+          return false;
+
+        if (LHS->getIntroduced().empty() && !RHS->getIntroduced().empty())
+          LHS->setIntroduced(Ctx, RHS->getIntroduced());
+
+        if (LHS->getDeprecated().empty() && !RHS->getDeprecated().empty()) {
+          LHS->setDeprecated(Ctx, RHS->getDeprecated());
+          if (LHS->getMessage().empty())
+            LHS->setMessage(Ctx, RHS->getMessage());
+          if (LHS->getReplacement().empty())
+            LHS->setReplacement(Ctx, RHS->getReplacement());
+        }
+
+        if (LHS->getObsoleted().empty() && !RHS->getObsoleted().empty()) {
+          LHS->setObsoleted(Ctx, RHS->getObsoleted());
+          if (LHS->getMessage().empty())
+            LHS->setMessage(Ctx, RHS->getMessage());
+          if (LHS->getReplacement().empty())
+            LHS->setReplacement(Ctx, RHS->getReplacement());
         }
 
-int clang_getCursorPlatformAvailability(CXCursor cursor,
-                                        int *always_deprecated,
+        return true;
+      });
+  AvailabilityAttrs.erase(It, AvailabilityAttrs.end());
+}
+
+int clang_getCursorPlatformAvailability(CXCursor cursor, int *always_deprecated,
                                         CXString *deprecated_message,
                                         int *always_unavailable,
                                         CXString *unavailable_message,
                                         CXPlatformAvailability *availability,
                                         int availability_size) {
   if (always_deprecated)
     *always_deprecated = 0;
   if (deprecated_message)
     *deprecated_message = cxstring::createEmpty();
   if (always_unavailable)
     *always_unavailable = 0;
   if (unavailable_message)
     *unavailable_message = cxstring::createEmpty();
 
   if (!clang_isDeclaration(cursor.kind))
     return 0;
 
   const Decl *D = cxcursor::getCursorDecl(cursor);
   if (!D)
     return 0;
 
-  return getCursorPlatformAvailabilityForDecl(D, always_deprecated,
-                                              deprecated_message,
-                                              always_unavailable,
-                                              unavailable_message,
-                                              availability,
-                                              availability_size);
+  SmallVector<AvailabilityAttr *, 8> AvailabilityAttrs;
+  getCursorPlatformAvailabilityForDecl(D, always_deprecated, deprecated_message,
+                                       always_unavailable, unavailable_message,
+                                       AvailabilityAttrs);
+  for (const auto &Avail :
+       llvm::enumerate(llvm::makeArrayRef(AvailabilityAttrs)
+                           .take_front(availability_size))) {
+    availability[Avail.index()].Platform =
+        cxstring::createDup(Avail.value()->getPlatform()->getName());
+    availability[Avail.index()].Introduced =
+        convertVersion(Avail.value()->getIntroduced());
+    availability[Avail.index()].Deprecated =
+        convertVersion(Avail.value()->getDeprecated());
+    availability[Avail.index()].Obsoleted =
+        convertVersion(Avail.value()->getObsoleted());
+    availability[Avail.index()].Unavailable = Avail.value()->getUnavailable();
+    availability[Avail.index()].Message =
+        cxstring::createDup(Avail.value()->getMessage());
+  }
+
+  return AvailabilityAttrs.size();
 }
-  
 void clang_disposeCXPlatformAvailability(CXPlatformAvailability *availability) {
   clang_disposeString(availability->Platform);
   clang_disposeString(availability->Message);
 }
 
 CXLanguageKind clang_getCursorLanguage(CXCursor cursor) {
   if (clang_isDeclaration(cursor.kind))
     return getDeclLanguage(cxcursor::getCursorDecl(cursor));