manmanren added a comment. Hi Erik,
Thanks for working on this! It is great to see these patches coming. Manman ================ Comment at: include/clang/Sema/Sema.h:9608 @@ -9604,1 +9607,3 @@ + /// \brief Whether we should emit an availability diagnostic for \c D. + bool ShouldDiagnoseAvailabilityOfDecl( ---------------- Can you explain the inputs and outputs? ================ Comment at: lib/Sema/SemaDeclAttr.cpp:6517 @@ +6516,3 @@ + +/// \brief This class implements -Wunguarded-availability. +class DiagnoseUnguardedAvailability ---------------- Can you add a high level description of what this class is doing to issue unguarded diagnostics? ================ Comment at: lib/Sema/SemaDeclAttr.cpp:6540 @@ +6539,3 @@ + + bool VisitTypeLoc(TypeLoc TL); + ---------------- Do we cover all usage of decls with the above three member functions? ================ Comment at: lib/Sema/SemaDeclAttr.cpp:6552 @@ +6551,3 @@ + + if (SemaRef.ShouldDiagnoseAvailabilityOfDecl(D, nullptr, false, ObjCPDecl, + ContextVersion, nullptr, AD)) { ---------------- It is easier to read if you add comment for the boolean argument. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:6554 @@ +6553,3 @@ + ContextVersion, nullptr, AD)) { + // All other diagnostic kinds have already been handled. + if (AD != Sema::AD_Partial) ---------------- Can you mention where they are handled? ================ Comment at: lib/Sema/SemaExpr.cpp:110 @@ -113,1 +109,3 @@ + VersionTuple ContextVersion, std::string *Message, + Sema::AvailabilityDiagnostic &AD) { ---------------- This looks like a refactoring of DiagnoseAvailabilityOfDecl. Is it possible to commit as a NFC patch? ================ Comment at: test/SemaObjC/unguarded-availability.m:93 @@ +92,3 @@ + label2: + goto label1; // expected-error{{cannot jump from this goto statement to its label}} + } ---------------- Should we emit error here? Or should we just ignore the tighter availability scope? https://reviews.llvm.org/D23003 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits