This is an archive of the discontinued LLVM Phabricator instance.

Add a fix-it for -Wunguarded-availability
ClosedPublic

Authored by arphaman on Apr 24 2017, 3:13 AM.

Details

Summary

This patch adds a fix-it for the -Wunguarded-availability warning. This fix-it is similar to the Swift one: it suggests that you wrap the statement in an if (@available) check. The produced fixits are indented (just like the Swift ones) to make them look nice in Xcode's fix-it preview.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Apr 24 2017, 3:13 AM
ahatanak edited edge metadata.Apr 24 2017, 1:55 PM

Do we want to enclose declaration statements in "if (@available)" too? I think doing so can cause compilation errors.

int x = function();

++x; // if declaration is wrapped, this is an error because x is undeclared in this scope.

Also, when function is used in a case clause, the whole case clause will be wrapped in "if (@available)". This won't cause a compilation error, but it's probably better to enclose only the statement of the case label.

switch (c) {
case1: t1 = function();
default: break;
}
arphaman updated this revision to Diff 96538.Apr 25 2017, 5:58 AM

Now the patch takes the following situations into account:

  • Enclose only the statement in a case.
  • If the fixit has to enclose a declaration statement, then the fixit will try to enclose the appropriate uses as well.

Hi Alex, thanks for working on this! This looks right, but I have a couple of comments.
Thanks,
Erik

lib/Sema/SemaDeclAttr.cpp
7151 ↗(On Diff #96538)

I think this could be simplified: If we iterate backwards through the CompoundStmt::body(), calling RecursiveASTVisitor::TraverseStmt(), and bail out if we encounter a DeclRefExpr to D, returning the current Stmt. This also has the advantage that we don't have to iterate forward through the CompoundStmt when we're looking for the last Child Stmt. Have you considered this option?

7249 ↗(On Diff #96538)

Since we emit a fixit to use __builtin_available in non objective-c modes, this diagnostic should take another argument to suggest using the right form of @available depending on the source language. (Currently it's hard-coded to suggest @available).

arphaman added inline comments.May 5 2017, 2:16 AM
lib/Sema/SemaDeclAttr.cpp
7151 ↗(On Diff #96538)

Good idea, I will change this code.

arphaman updated this revision to Diff 97922.May 5 2017, 3:17 AM
arphaman marked 2 inline comments as done.
  • Simplify the RecursiveASTVisitor as suggested by Erik
  • Improve the note to include __builtin_available
erik.pilkington accepted this revision.May 5 2017, 8:46 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 5 2017, 8:46 AM
This revision was automatically updated to reflect the committed changes.