aaron.ballman added inline comments.
================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6117 + // to ensure that the variable is 'const' so that we can error on + // modification, which can otherwise overrelease. + VD->setType(Ty.withConst()); ---------------- erik.pilkington wrote: > aaron.ballman wrote: > > erik.pilkington wrote: > > > aaron.ballman wrote: > > > > overrelease -> over-release > > > > > > > > Btw, this isn't a bit of a hack, it's a huge hack, IMO. Instead, can we > > > > simply err if the user hasn't specified `const` explicitly? I'm not a > > > > fan of "we're hiding this rather important language detail behind an > > > > attribute that can be ignored". This is especially worrisome because it > > > > means the variable is const only when ARC is enabled, which seems like > > > > surprising behavioral differences for that compiler flag. > > > An important part of this feature is that it could applied to parameters > > > with `#pragma clang attribute` over code that has been audited to be > > > safe. If we require adding `const` to every parameter, then that falls > > > apart :/. > > > > > > For better or for worse, this is also consistent with other cases of > > > pseudo-strong in the language, i.e.: > > > ``` > > > void f(NSArray *A) { > > > for (id Elem in A) > > > Elem = nil; // fine in -fno-objc-arc, error in -fobjc-arc because > > > Elem is implicitly const. > > > } > > > ``` > > > An important part of this feature is that it could applied to parameters > > > with #pragma clang attribute over code that has been audited to be safe. > > > If we require adding const to every parameter, then that falls apart :/. > > > > Thanks for letting me know about that use case; it adds some helpful > > context. However, I don't see why this falls apart -- if you're auditing > > the code, you could add the `const` qualifiers at that time, couldn't you? > > > > Alternatively, if we warned instead of erred on the absence of `const`, > > then this could be automated through clang-tidy by checking declarations > > that are marked with the attribute but are not marked `const` and use the > > fix-it machinery to update the code. > > > > > For better or for worse, this is also consistent with other cases of > > > pseudo-strong in the language, > > > > Yes, but it's weird behavior for an attribute. The attribute is applied to > > the *declaration* but then it silently modifies the *type* as well, but > > only for that one declaration (which is at odds with the usual rules for > > double-square bracket attributes and what they appertain to based on > > syntactic location). Sometimes, this will lead to good behavior (such as > > semantic checks and when doing AST matching over const-qualified types) and > > sometimes it will lead to confusing behavior (pretty-printing the code is > > going to stick const qualifers where none are written, diagnostics will > > talk about const qualifiers that aren't written in the source, etc). > > > > Also, doesn't this cause ABI issues? If you write the attribute on a > > parameter, the presence or absence of that attribute is now part of the ABI > > for the function call because the parameter type will be mangled as being > > const-qualified. (Perhaps that's more of a feature than a bug -- I imagine > > you want both sides of the ABI to agree whether ARC is enabled or not?) > > > > I'm wondering if this is weird enough that it should be using a keyword > > spelling instead of an attribute spelling? However, I'm not certain if that > > works with `#pragma clang attribute`. > > Thanks for letting me know about that use case; it adds some helpful > > context. However, I don't see why this falls apart -- if you're auditing > > the code, you could add the const qualifiers at that time, couldn't you? > > If we're going to require O(n) annotations for each application, then there > isn't really any reason to use the automatic #pragma annotations. I think it > would be nice to be able to throw this around more easily, so you could see > what parts of your program are worth auditing, and what stuff breaks. I don't > think the const annotation is a dealbreaker for me, but I think it makes this > feature easier to use and more consistent. > > > Yes, but it's weird behavior for an attribute. The attribute is applied to > > the *declaration* but then it silently modifies the *type* as well, but > > only for that one declaration (which is at odds with the usual rules for > > double-square bracket attributes and what they appertain to based on > > syntactic location). Sometimes, this will lead to good behavior (such as > > semantic checks and when doing AST matching over const-qualified types) and > > sometimes it will lead to confusing behavior (pretty-printing the code is > > going to stick const qualifers where none are written, diagnostics will > > talk about const qualifiers that aren't written in the source, etc). > > Well, we would get the bad diagnostics and stuff about const where none was > written with a keyword that implied `const` as well. I don't think that the > standard double square bracket attribute rules really matter here, since we > don't really follow them for clang extensions. I guess it boils down to the > philosophical question: what should an attribute do? I agree that adding > const is a bit strange, but I don't think its beyond the pale. > > > Also, doesn't this cause ABI issues? If you write the attribute on a > > parameter, the presence or absence of that attribute is now part of the ABI > > for the function call because the parameter type will be mangled as being > > const-qualified. (Perhaps that's more of a feature than a bug -- I imagine > > you want both sides of the ABI to agree whether ARC is enabled or not?) > > Top-level cv qualifiers on parameters are ignored in the mangling, so the > parameter isn't mangled as const qualified, I'll add a testcase for this. > > > I'm wondering if this is weird enough that it should be using a keyword > > spelling instead of an attribute spelling? However, I'm not certain if that > > works with #pragma clang attribute. > > That's not the end of the world, we could just add a new custom pragma if we > went with a keyword. I don't think we want to put this in the type system, > since I think it only really makes sense to use this with declarations of > parameters and variables. AFAIK an attribute is the only way to uniformly add > something to every kind of declaration that we want to support (objc method > parameter, variable, parameter), so I think it's the best way to expose this. > Well, we would get the bad diagnostics and stuff about const where none was > written with a keyword that implied const as well. True, but at the same time, the keyword is a stronger signal to the user that this is something special that behaves strangely, too. > I don't think that the standard double square bracket attribute rules really > matter here, since we don't really follow them for clang extensions. I guess > it boils down to the philosophical question: what should an attribute do? I > agree that adding const is a bit strange, but I don't think its beyond the > pale. We typically do follow the appertainment rules for extensions and this feature kind of muddies that because the attribute appertains to both the declaration and the type in a way. But, as you say, this is an extension and we can be weird (on reflection, there is some precedent here too -- calling conventions come to mind as they're sort of type attributes and sort of declaration attributes). > Top-level cv qualifiers on parameters are ignored in the mangling, so the > parameter isn't mangled as const qualified, I'll add a testcase for this. That's a compelling point, thank you for pointing that out! > ...so I think it's the best way to expose this. Okay, I'll hold my nose on the const qualifier then; there's enough reasons to do it, and "ewwwww" isn't a strong enough reason not to do it. ================ Comment at: clang/test/SemaObjC/externally-retained.m:2 +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0 -fobjc-runtime=macosx-10.13.0 -fblocks -fobjc-arc %s -verify +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0 -fobjc-runtime=macosx-10.13.0 -fblocks -fobjc-arc -xobjective-c++ %s -verify + ---------------- Can you add a test that runs with ARC disabled to test out the `LangOpts` behavior from Attr.td? (Can be done in a separate test file if you want, too.) ================ Comment at: clang/test/SemaObjC/externally-retained.m:10 +void test1() { + EXT_RET int a; // expected-warning{{'objc_externally_retained' can only be applied to}} + EXT_RET __weak ObjCTy *b; // expected-warning{{'objc_externally_retained' can only be applied to}} ---------------- Can you add a test that demonstrates the attribute accepts no args? ================ Comment at: clang/test/SemaObjC/externally-retained.m:14 + + EXT_RET int (^d)() = ^{return 0;}; + EXT_RET ObjCTy *e = 0; ---------------- Should this be useful for function pointer parameters as well? e.g., ``` typedef void (*fp)(EXT_RET __strong ObjCTy *); void f(__strong ObjCTy *); void g(EXT_RET ObjCTy *Ptr) { fp Fn = f; // Good idea? Bad idea? Fn(Ptr); // Which behavior "wins" in this call? } ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55865/new/ https://reviews.llvm.org/D55865 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits