Page MenuHomePhabricator

unguarded availability: add a fixit for the "annotate '...' with an availability attribute to silence" note

Authored by arphaman on Jul 21 2017, 7:58 AM.

Diff Detail


Event Timeline

arphaman created this revision.Jul 21 2017, 7:58 AM

Thanks for working on this! This looks like it would be very useful.

7230 ↗(On Diff #107674)

Its unfortunate to loop over every macro. Can we use Preprocessor::getMacroDefinition()?

7231 ↗(On Diff #107674)

It would be nice if we could recommend using this macro even if it isn't defined, as users might not have included the <os/availability.h> header. Maybe we can do that on apple platforms, noting that the the macro is declared in os/availability.h if it isn't already defined?

7238 ↗(On Diff #107674)

OffendingDecl, right? Thats the one with the offending availability attribute

7240 ↗(On Diff #107674)

I was somewhat uncertain about adding a fixit for this because its difficult to determine where exactly the availability attribute should go, it looks like this doesn't emit an attribute at the right place for this, for example. (It should go after the meth)

@interface X

Maybe we can do better if we look at what Enclosing actually declares? It's not worth it to emit an incorrect fixit.

arphaman marked 3 inline comments as done.Jul 24 2017, 7:41 AM
arphaman added inline comments.
7230 ↗(On Diff #107674)

Good call.

7231 ↗(On Diff #107674)

I think we either have to go the full way (i.e. have an #include <os/availability.h> fixit inserted as well) or just avoid any fixits. Note that these fixits will be used pretty much only in Xcode which doesn't show any notes that don't have fixits, so an additional note wouldn't make sense. We could potentially change the message of note_partial_availability_silence but I doubt it's that useful.

7240 ↗(On Diff #107674)

Yeah, Good point.

arphaman updated this revision to Diff 107903.Jul 24 2017, 7:42 AM
arphaman marked 2 inline comments as done.

Addressed Erik's comments

erik.pilkington accepted this revision.Jul 24 2017, 9:23 AM

LGTM, thanks for working on this!

7230 ↗(On Diff #107674)

Looks like there is a convenience function: isMacroDefined(StringRef)

7231 ↗(On Diff #107674)

Ok, I think this is fine.

7063 ↗(On Diff #107903)

TagDecls also have a special rule: struct __attribute__(...) X;.

This revision is now accepted and ready to land.Jul 24 2017, 9:23 AM
This revision was automatically updated to reflect the committed changes.
arphaman marked 2 inline comments as done.