This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Use dotted form of macOS version for unguarded availability FixIts
ClosedPublic

Authored by jkorous on May 11 2018, 4:45 AM.

Details

Summary

E. g. use "10.11" instead of "10_11".

This is just a consistency fix - the dotted format is already used everywhere else.

rdar://problem/39845032

Diff Detail

Event Timeline

jkorous created this revision.May 11 2018, 4:45 AM

That test passes for me without this patch applied, why is UsesUnderscores actually getting set in the radar?

jkorous planned changes to this revision.May 11 2018, 8:47 AM

Sorry, my bad. I tried to get rid of dependency on Foundation.h and didn't check the test is relevant for the fix after that.

#import <Foundation/Foundation.h>

@interface foo
- (void) method NS_AVAILABLE_MAC(10_12);
@end

int main() {
    [foo method];
    return 0;
}

Looks like NS_AVAILABLE_MAC is not equivalent to the attribute.

Interesting question about UsesUnderscores, thanks! It looks like it must be set through constructor somewhere - will figure this out.

jkorous updated this revision to Diff 146605.May 14 2018, 7:29 AM

Fixed the test. It turned out that version component separator for availability is set during parsing which is why all other tests (including my bad one) are passing.

The only way how to set underscore as a separator is through VersionTuple constructor...

https://clang.llvm.org/doxygen/classclang_1_1VersionTuple.html#a71e5354ccb617494136bdcb4d208a573

clang::VersionTuple::VersionTuple	(	unsigned 	Major,
  unsigned 	Minor,
  unsigned 	Subminor,
  unsigned 	Build,
  bool 	UsesUnderscores = false 
)

...which is exactly what is happening during parsing version tuple...

VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
  // ...
  return VersionTuple(Major, Minor, Subminor, (AfterMajorSeparator == '_'));
}

...which function is called when parsing availability attribute:

void Parser::ParseAvailabilityAttribute(IdentifierInfo &Availability,
  // ...
  VersionTuple Version = ParseVersionTuple(VersionRange);
  // ...
}

I am not 100% sure it's the best thing to set printing style at the point of parsing version tuples but I am not sure it's bad either. Unless someone convinces me otherwise I would rather not do any major changes.

erik.pilkington accepted this revision.May 14 2018, 8:23 AM

Thanks for the explanation, LGTM.

This revision is now accepted and ready to land.May 14 2018, 8:23 AM
jkorous planned changes to this revision.May 15 2018, 10:19 AM

Sorry for me being dense here - since the output format is determined by input source code there's more work to do.

I tried to change format of introduced version in couple of tests and the output mirrors the change. That basically means that whenever someone uses underscore

__attribute__((availability(macosx, introduced = 10_12)))

instead of dot

__attribute__((availability(macosx, introduced = 10.12)))

Warnings and FixIts get underscore as well.

I am now thinking about just switching to dot format in ParseAvailabilityAttribute() because it's much simpler to maintain consistency that way.

I am now thinking about just switching to dot format in ParseAvailabilityAttribute() because it's much simpler to maintain consistency that way.

Okay, but if we're going to treat underscores as dots while printing diagnostics and fixits (which I think we should), then we shouldn't track the spelling in VersionTuple: we no longer have any use for it. The only reason that VersionTuple supports this is because of r219124, which was written so that diagnostic printing would be consistent with how the attribute was spelled. Maybe there was a good reason for this, but I can't see the radar that was linked in the commit message. Can you look up that radar to see what r219124 was actually trying to fix? I don't think we should move forward with this until we have that context.

Thanks!
Erik

Eric, thanks for the context. I am now clarifying internally and if possible would like to simplify the whole format business by using just one delimiter everywhere. Would that make sense to you or do you think we should respect delimiter used in sources?

jkorous updated this revision to Diff 147184.May 16 2018, 2:36 PM

After some internal discussion we agreed that we can simplify things and get consistent behaviour by using dot everywhere in diagnostics.

This is pretty much revert of r219124 + update of tests.

This revision is now accepted and ready to land.May 16 2018, 2:36 PM
erik.pilkington accepted this revision.May 16 2018, 2:39 PM

Great, LGTM!

This revision was automatically updated to reflect the committed changes.