[PATCH] D26227: Don't require nullability on 'va_list'.
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. I don't see a better way to do this. Nice meta programming hack. Repository: rL LLVM https://reviews.llvm.org/D26227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26109: Warn when 'assume_nonnull' infers nullability within an array.
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM! Repository: rL LLVM https://reviews.llvm.org/D26109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26108: Add -Wnullability-completeness-on-arrays.
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. Looks good! I appreciate the refactoring of recordNullabilitySeen Repository: rL LLVM https://reviews.llvm.org/D26108 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25850: Accept nullability annotations (_Nullable) on array parameters
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. This is looking great, Jordan. Repository: rL LLVM https://reviews.llvm.org/D25850 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26226: Don't require nullability on template parameters in typedefs.
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. Seem fine; I'd rename the "FIXME" to a "Note" unless you're going to pass a flag to Type::canHaveNullability to say how to classify dependent types. Repository: rL LLVM https://reviews.llvm.org/D26226 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24059: NFC: refactor applyObjCProtocolQualifiers from SemaType.cpp to ASTContext so it can be shared.
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. Yep, this refactor looks good! https://reviews.llvm.org/D24059 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23080: ObjC: Use a new type for ObjC type parameter (patch 3 out of 3)
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. Ahhh, much cleaner. Thanks! https://reviews.llvm.org/D23080 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23079: ObjC: Use a new type for ObjC type parameter (patch 2 out of 3)
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. This looks great, thank you! https://reviews.llvm.org/D23079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. I don't love the fact that it makes callers fragile, but having to do tentative parsing for these otherwise-unambiguous cases is expensive (in compile time). We should only use tentative parsing when we have an actual ambiguity to resolve. https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
doug.gregor added a comment. This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out? https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23079: ObjC: Use a new type for ObjC type parameter (patch 2 out of 3)
doug.gregor added inline comments.
Comment at: include/clang/AST/RecursiveASTVisitor.h:1037
@@ -1036,1 +1036,3 @@
+DEF_TRAVERSE_TYPE(ObjCTypeParamType, {})
+
manmanren wrote:
> doug.gregor wrote:
> > I'm sorta shocked that we don't visit the protocol qualifiers here, but I
> > guess we haven't been doing that for ObjCObjectType all along. Weird.
> Right below, we don't visit the protocol qualifiers for ObjCObjectType :]
> If you think we should, I can patch it up for both types.
Yes, I think we should.
Comment at: include/clang/AST/Type.h:4786
@@ +4785,3 @@
+ bool isSugared() const { return false; }
+ QualType desugar() const { return QualType(this, 0); }
+
manmanren wrote:
> doug.gregor wrote:
> > This is an interesting choice. Objective-C type parameters were treated
> > like typedefs before, so they always act like their underlying type (e.g.,
> > because they are typedef name declarations). Why isn't ObjCTypeParamType
> > sugar for the underlying type of the type parameter (I.e., the bound) w/
> > the protocol qualifiers?
> Are you suggesting to canonicalize ObjCTypeParamType to the underlying type
> with the protocol qualifiers as well? Or just desugaring?
>
> I think I have tried to set ObjCTypeParamType as NON_CANONICAL_TYPE in
> TypeNodes.def, but had some issues.
Canonicalize to the underlying type + protocol qualifiers. We don't need the
desugaring part.
https://reviews.llvm.org/D23079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23080: ObjC: Use a new type for ObjC type parameter (patch 3 out of 3)
doug.gregor requested changes to this revision.
doug.gregor added a comment.
This revision now requires changes to proceed.
I think most of the complexity here will fold away when ObjCTypeParamType
becomes sugar for the underlying ObjCObjectPointerType.
Comment at: lib/AST/Type.cpp:1095
@@ +1094,3 @@
+return ctx.getQualifiedType(argType, splitType.Quals);
+ // Apply protocol lists if exists. Should we combine protocol list?
+ if (const auto *objcPtr = dyn_cast(argType)) {
I think we should combine the protocol lists.
Comment at: lib/AST/Type.cpp:1290
@@ +1289,3 @@
+/// in @implementation. @implementation does not take type parameters.
+QualType QualType::handleObjCTypeParamType(ASTContext ) const {
+ return simpleTransform(ctx, *this,
I think you won't need this if you take the approach I suggested on the
previous patch of making ObjCTypeParamType type sugar for, effectively, the
type you're computing here.
Comment at: lib/Sema/SemaExprObjC.cpp:388
@@ +387,3 @@
+ if (auto *TPT = T->getAs())
+T = TPT->getDecl()->getUnderlyingType();
+
Dropping protocol qualifiers here? Again, I think this would be solved by
treating ObjCTypeParamType as sugar.
Comment at: lib/Sema/SemaExprObjC.cpp:824
@@ -820,1 +823,3 @@
+(!isa(PtrT->getPointeeType()) &&
+ !Context.hasSameUnqualifiedType(PtrT->getPointeeType(), IdT))) {
Diag(SR.getBegin(), diag::err_objc_literal_method_sig)
Also unnecessary w/ the desugaring approach?
Comment at: lib/Sema/SemaType.cpp:901
@@ -900,2 +900,3 @@
// Objective-C object pointer types must be substitutable for the bounds.
-if (const auto *typeArgObjC = typeArg->getAs()) {
+if (typeArg->getAs() ||
+isa(typeArg)) {
... same comment about desugaring.
Comment at: lib/Sema/SemaType.cpp:917
@@ +916,3 @@
+ const ObjCObjectPointerType *typeArgObjC = nullptr;
+ if (isa(typeArg)) {
+typeArgObjC = typeArg->getAs()->getDecl()->
Here too :)
Comment at: lib/Sema/SemaType.cpp:3397
@@ -3365,2 +3396,3 @@
// Look at Objective-C object pointers.
-if (auto objcObjectPtr = type->getAs()) {
+if (type->getAs() ||
+isa(type)) {
... same comment about desugaring.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:63
@@ -62,2 +62,3 @@
- if (T->isAnyPointerType() || T->isReferenceType())
+ if ((T->isAnyPointerType() && !isa(T)) ||
+ T->isReferenceType())
... same comment about desugaring.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:133
@@ -131,1 +132,3 @@
static bool isPointerToConst(QualType Ty) {
+ if (isa(Ty))
+return false;
... same comment about desugaring.
https://reviews.llvm.org/D23080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23079: ObjC: Use a new type for ObjC type parameter (patch 2 out of 3)
doug.gregor requested changes to this revision.
doug.gregor added a comment.
This revision now requires changes to proceed.
A couple of comments above, but this is looking very good.
Comment at: include/clang/AST/RecursiveASTVisitor.h:1037
@@ -1036,1 +1036,3 @@
+DEF_TRAVERSE_TYPE(ObjCTypeParamType, {})
+
I'm sorta shocked that we don't visit the protocol qualifiers here, but I guess
we haven't been doing that for ObjCObjectType all along. Weird.
Comment at: include/clang/AST/RecursiveASTVisitor.h:1270
@@ -1267,1 +1269,3 @@
+DEF_TRAVERSE_TYPELOC(ObjCTypeParamType, {})
+
Same shock at not visiting the protocol qualifiers here :)
Comment at: include/clang/AST/Type.h:4786
@@ +4785,3 @@
+ bool isSugared() const { return false; }
+ QualType desugar() const { return QualType(this, 0); }
+
This is an interesting choice. Objective-C type parameters were treated like
typedefs before, so they always act like their underlying type (e.g., because
they are typedef name declarations). Why isn't ObjCTypeParamType sugar for the
underlying type of the type parameter (I.e., the bound) w/ the protocol
qualifiers?
Comment at: include/clang/AST/TypeLoc.h:696
@@ -695,1 +695,3 @@
+struct ObjCTypeParamTypeLocInfo {
+ SourceLocation ProtocolLAngleLoc;
The angle bracket locations don't exist if you don't have any protocols, so you
could conceivably put them into the extra local data.
Comment at: include/clang/Serialization/ASTBitCodes.h:906
@@ -906,1 +905,3 @@
+ TYPE_PIPE = 43,
+ TYPE_OBJC_TYPE_PARAM = 44
};
Comment please, even if it's trivial.
Comment at: lib/AST/ItaniumMangle.cpp:2916
@@ +2915,3 @@
+ }
+ mangleSourceName(T->getDecl()->getIdentifier());
+}
This doesn't look right. Typedef names aren't mangled; we should mangle based
on building the ObjCPointerType with the canonical type of
T->getDecl()->getUnderlyingType() w/ the protocol qualifiers added to it.
That said, I don't think this case will ever come up, because you can't define
something that would C++ mangling within an Objective-C @interface.
Comment at: lib/AST/MicrosoftMangle.cpp:2318
@@ +2317,3 @@
+ SourceRange Range) {
+ mangleType(T->getDecl()->getUnderlyingType(), Range);
+}
I'd expect this to have the same implementation as the Itanium one: the
canonical type w/ the underlying type + protocol qualifiers. Again, I don't
think this code path can ever get triggered.
https://reviews.llvm.org/D23079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23078: ObjC: Use a new type for ObjC type parameter (Patch 1 out of 3)
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. This refactor LGTM. https://reviews.llvm.org/D23078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D22392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22248: [Sema] Create a separate group for incompatible function pointer warning
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D22248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators
doug.gregor added a comment.
A bunch of comments above. This needs much more extensive testing, because
there are numerous paths through the ternary operator code and the results need
to be symmetric.
Comment at: lib/Sema/SemaExpr.cpp:7007
@@ +7006,3 @@
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+ ASTContext ) {
This name could be improved. You're not really 'modifying' nullability here; in
the general case, you're computing the appropriate nullability given the LHS,
RHS, and applying that to the result type.
Comment at: lib/Sema/SemaExpr.cpp:7024
@@ +7023,3 @@
+
+ NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+
I'm fairly certain that more extensive testing will show that you need to have
the LHSExpr as well, to look at the nullability of both.
Comment at: lib/Sema/SemaExpr.cpp:7030
@@ +7029,3 @@
+ // Create a new AttributedType with the new nullability kind.
+ QualType NewTy = ResTy.getDesugaredType(Ctx);
+ auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);
It would be better to only unwrap sugar until we hit the
nullability-attributed type, then replace it.
Comment at: test/Sema/nullability.c:137
@@ +136,3 @@
+
+ int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
You really need much more testing coverage here, e.g., for ternary operators
where the types on the second and third arguments are different types (say,
superclass/subclass pointer), the nullability is on either argument, etc. The
ternary operator, especially in C++, has a ton of different cases that you need
to look at.
https://reviews.llvm.org/D22392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22391: [Sema] Add warning for implicitly casting a null constant to a non null pointer type
doug.gregor added a comment. I think this check should go into SemaChecking.cpp https://reviews.llvm.org/D22391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. Yes, that's a LGTM, sorry for being unclear. http://reviews.llvm.org/D20383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers
doug.gregor added a comment. Yeah, this looks like the right approach. PCH follows the same rules as modules when it comes to newer information shadowing imported information. http://reviews.llvm.org/D20383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20451: [Parser] Fix look ahead after EOF while parsing objc message and lambdas
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM, sorry for the delay! http://reviews.llvm.org/D20451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19679: Method pool in modules: sync up out of date selectors before writing the module
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM! http://reviews.llvm.org/D19679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19679: Method pool in modules: sync up out of date selectors before writing the module
doug.gregor added a comment. Mostly looks good, but see my comment about MapVector invalidation. Comment at: lib/Serialization/ASTWriter.cpp:4394 @@ +4393,3 @@ + // date, so we need to pull in the new content here. + for (auto : SelectorIDs) +SemaRef.updateOutOfDateSelector(SelectorAndID.first); Are we certain that SelectorIDs cannot get updated during this iteration under any circumstances? If the underlying vector were to get reallocated, we'd end up with a horrible-to-reproduce use-after-free here. It might be worth copying SelectorIDs or indexing by an integer value rather than using the for-each loop. http://reviews.llvm.org/D19679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19443: Module Debugging: Fix the condition for determining whether a template instantiation is in a module.
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. Yeah, this LGTM. Repository: rL LLVM http://reviews.llvm.org/D19443 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18823: Implementation of VlA of GNU C++ extension
doug.gregor added a comment. I think it's completely reasonable to implement support for VLAs as a GNU C++ extension. We did go through a phase where we tried to avoid implementing VLAs in C++ because we considered them to be a poor feature in C++. However, their use was wide-spread enough that we changed course and enabled the implementation for POD types in C++. That got us most of the compatibility without a significant amount of effort, whereas we didn't have the infrastructure to handle non-PODs at that time. It wasn't a statement of intent---it just wasn't important enough to implement at the time. Looks like rjmccall's work on VLAs containing ARC-qualified pointers got us most of the way there, so it makes sense to generalize it to non-POD types. http://reviews.llvm.org/D18823 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18709: Add copyright notice to modulemap files
doug.gregor added a subscriber: doug.gregor. doug.gregor accepted this revision. doug.gregor added a reviewer: doug.gregor. doug.gregor added a comment. This revision is now accepted and ready to land. Yes, this is acceptable. Please go ahead and commit. http://reviews.llvm.org/D18709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15314: Fix a bug in unavailable checking
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM http://reviews.llvm.org/D15314 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17355: [Sema] Remove assert in TreeTransform::TransformObjCObjectType
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM, sorry for the delay. http://reviews.llvm.org/D17355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15314: Fix a bug in unavailable checking
doug.gregor added a comment. The approach and patch look okay to me, but can we give "UnavailableCheck" a less ambiguous name? For example, "TreatUnavailableAsInvalid"? http://reviews.llvm.org/D15314 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16906: [Parser] Perform CachedTokens update dependent on token consume request
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. LGTM! http://reviews.llvm.org/D16906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15173: [Preprocessor] Fix assertion in AnnotatePreviousCachedTokens
doug.gregor accepted this revision. doug.gregor added a comment. This approach looks great to me. http://reviews.llvm.org/D15173 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15729: Load compiler plugins in ASTUnit, too
doug.gregor added a comment. It looks good. Are you able to include a test for this? http://reviews.llvm.org/D15729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15490: [libclang] Add a flag to create the precompiled preamble on the first parse.
doug.gregor added a comment. For reference, you can test this by setting the environment variable LIBCLANG_TIMING in your test, and checking that the string "Precompiling preamble" shows up on first parse. http://reviews.llvm.org/D15490 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
