[PATCH] D22171: [ObjC Availability] Implement parser support for Objective-C's @available

[Manman Ren via cfe-commits]

manmanren added a comment.

Thanks for working on this, Erik.

Manman


================
Comment at: include/clang/AST/Availability.h:32
@@ +31,3 @@
+class AvailabilitySpec {
+  VersionTuple Version;
+  StringRef Platform;
----------------
Can you put a description for "Version"? i.e if it represents a range, specify the range here.

================
Comment at: include/clang/AST/Availability.h:42
@@ +41,3 @@
+
+  // Constructor for '*'
+  AvailabilitySpec(SourceLocation StarLoc)
----------------
Can you make this comment a full sentence?

================
Comment at: include/clang/AST/Availability.h:51
@@ +50,3 @@
+
+  // true when this represents the '*' case.
+  bool isOtherPlatformSpec() const { return Version.empty(); }
----------------
Same here.

================
Comment at: include/clang/Basic/VersionTuple.h:29
@@ +28,3 @@
+
+  unsigned UsesUnderscores : 1;
+
----------------
If this changes in this file are not related to @available, please commit it separately.

================
Comment at: lib/AST/StmtPrinter.cpp:502
@@ +501,3 @@
+    ObjCAvailabilityCheckExpr *Node) {
+  OS << "@available(...)";
+}
----------------
Why not print other information of Node?

================
Comment at: lib/Parse/ParseDecl.cpp:723
@@ -722,3 +722,3 @@
 VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
-  Range = Tok.getLocation();
+  Range.setBegin(Tok.getLocation());
 
----------------
I don't quite get what motivates this change. Are we not setting the range correctly before, for version tuples?

================
Comment at: lib/Parse/ParseExpr.cpp:2875
@@ +2874,3 @@
+
+/// Validate availability spec list, emitting diagnostics if necessary.
+static bool CheckAvailabilitySpecList(Parser &P,
----------------
Please comment on the return value, i.e return true if invalid.

================
Comment at: lib/Parse/ParseExpr.cpp:2957
@@ +2956,3 @@
+  bool HasError = false;
+  for (;;) {
+    Optional<AvailabilitySpec> Spec = ParseAvailabilitySpec();
----------------
Why use "for (;;)" instead of "while(true)"?

================
Comment at: lib/Parse/ParseExpr.cpp:2964
@@ +2963,3 @@
+
+    if (!TryConsumeToken(tok::comma))
+      break;
----------------
Can you verify that when having error parsing a single spec, the compiler can still reasonably continue? Should we skip until the next comma, or the next right paren? 

================
Comment at: lib/Parse/ParseObjc.cpp:2863
@@ +2862,3 @@
+      ConsumeToken();
+      return ParseAvailabilityCheckExpr(AtLoc);
+    default: {
----------------
Should we move ConsumeToken to inside ParseAvailabilityCheckExpr, or at least provide a comment on skipping "@available", to be consistent with other parsing functions?

================
Comment at: lib/Parse/ParseObjc.cpp:2865
@@ +2864,3 @@
+    default: {
+      const char *str = nullptr;
+      if (GetLookAheadToken(1).is(tok::l_brace)) {
----------------
Is this a format change for "default:"? If yes, can you commit it before this @available change?


http://reviews.llvm.org/D22171