This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rdwampler on May 23 2017, 7:56 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

rdwampler created this revision.May 23 2017, 7:56 PM
arphaman added inline comments.Jun 2 2017, 7:33 AM
tools/libclang/CIndex.cpp
7287 ↗(On Diff #99979)

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.

7322 ↗(On Diff #99979)

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 added inline comments.Jun 2 2017, 9:36 AM
tools/libclang/CIndex.cpp
7322 ↗(On Diff #99979)

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 added inline comments.Jun 5 2017, 10:22 AM
tools/libclang/CIndex.cpp
7322 ↗(On Diff #99979)

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 updated this revision to Diff 101569.Jun 6 2017, 8:34 AM

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

arphaman edited edge metadata.Jun 6 2017, 11:08 AM

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 ↗(On Diff #101569)

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

tools/libclang/CIndex.cpp
7239 ↗(On Diff #101569)

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

7255 ↗(On Diff #101569)

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

rdwampler updated this revision to Diff 101787.Jun 7 2017, 12:00 PM
rdwampler marked 6 inline comments as done.

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

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

tools/libclang/CIndex.cpp
7262 ↗(On Diff #101787)

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

7268 ↗(On Diff #101787)

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 marked an inline comment as done.Jun 7 2017, 5:39 PM
rdwampler added inline comments.
test/Index/availability.c
20 ↗(On Diff #101787)

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
7268 ↗(On Diff #101787)

Agreed.

arphaman added inline comments.Jun 8 2017, 10:12 AM
test/Index/availability.c
20 ↗(On Diff #101787)

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 updated this revision to Diff 102050.Jun 9 2017, 10:21 AM

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

This revision is now accepted and ready to land.Jun 9 2017, 11:06 AM

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

rdwampler marked 2 inline comments as done.Jun 9 2017, 11:16 AM

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

This revision was automatically updated to reflect the committed changes.

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

Alex, I will look into it. Thanks!

rdwampler updated this revision to Diff 102150.Jun 11 2017, 7:39 PM

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.

Thanks, I'll recommit today.

Recommitted in r305221.