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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
