[cfe-commits] Preliminary patch to support MSVC __declspec(property)
John McCall
rjmccall at apple.com
Mon Apr 15 15:24:40 PDT 2013
On Apr 15, 2013, at 7:35 AM, endlessroad1991 at gmail.com wrote:
> Rebased on r179523
+ bool isDeclspecPropertyAttribute() const {
+ return IsProperty && getKind() == AT_MsProperty && SyntaxUsed == AS_Declspe
+ }
+
In what ways is this more than just IsProperty?
+ if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())
+ Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
+ diag::err_invalid_thread)
+ << DeclSpec::getSpecifierName(TSCS);
Indentation.
+ if (II) Loc = D.getIdentifierLoc();
What's with checks like these? When can you make an anonymous property
declaration? I think you should just be checking this early and then bailing out
if you don't have a name.
+
+ case Expr::MSPropertyRefExprClass:
+ return CT_Can;
This case should assert with llvm_unreachable. If we're processing a bare
MSPropertyRefExpr for whether it can throw, we've done something wrong.
> On Tue, Apr 9, 2013 at 7:27 AM, John McCall <rjmccall at apple.com> wrote:
> On Apr 1, 2013, at 6:00 AM, endlessroad1991 at gmail.com wrote:
> > Rebased on r178459
> >
> > All original tests still pass, and 2 new tests(one for Parser, one for Sema).
>
> Major cross-cutting problem: none of the places where you're manually
> calling CheckPlaceholderExpr should be necessary. I'm positive that
> we're already filtering out placeholders in these places; if we weren't,
> we'd have a serious bug that would hit a ton of Objective-C code.
>
> The other nice thing about relying on the existing machinery for this is
> that you wouldn't have a ton of latent bugs about not looking through parens. :)
> Yes, I'm aware of that :-)
> I will explain each case of CheckPlaceholderExpr() in the last.
>
>
> SemaTemplateInstantiateDecl.cpp:
>
> + if (DI->getType()->isInstantiationDependentType() ||
> + DI->getType()->isVariablyModifiedType()) {
>
> Why are you checking for variably-modified types here?
> I just copied most of handleField() into handleMSProperty(),
> In the attached new patch, it will complain about VariablyModifiedType
Ah, sure, you definitely do need to detect this.
> SemaDeclCXX.cpp:
>
> +static AttributeList *getMSPropertyAttr(AttributeList *list) {
> + for (AttributeList* it = list; it != 0; it = it->getNext())
> + if (it->getName()->getName() == "property")
>
> if (it->getKind() == AttributeList::AT_property)
>
> I'm a little bothered by doing this here instead of filtering it out beforehand,
> but it's probably okay; attributes on fields are relatively uncommon.
> I don't like this either, but ParsedAttr are only attached to FieldDecl after it's created...
> And here, we need to judge whether we should create FieldDecl or MSPropertyDecl.
> So I just put it here.
> Any ideas?
I think it's fine.
> AttributeList.cpp:
>
> - if (ScopeName)
> - Buf += ScopeName->getName();
> - // Ensure that in the case of C++11 attributes, we look for '::foo' if it is
> - // unscoped.
> - if (ScopeName || SyntaxUsed == AS_CXX11)
> - Buf += "::";
> + // For property, "ScopeName" is used to store getter function name
> + if (AttrName != "property") {
> + if (ScopeName)
> + Buf += ScopeName->getName();
> + // Ensure that in the case of C++11 attributes, we look for '::foo' if it is
> + // unscoped.
> + if (ScopeName || SyntaxUsed == AS_CXX11)
> + Buf += "::";
> + }
>
> Okay, this is actually a hot enough path that we'd really rather not introduce
> a string compare into it. Figure out some other way to store the second
> string; you could throw things like MessageExpr into a union.
> Done. Turns out that this problem has been encountered before: availability and typetag attributes.
> I used the same mechanism as existing code: malloc more than sizeof(AttributeList), and use the extra space.
Sure, good call.
>
> + Expr* getBaseExpr() const { return BaseExpr; }
> + void setBaseExpr(Expr *baseExpr) { BaseExpr = baseExpr; }
> + MSPropertyDecl* getMSPropertyDecl() const { return Decl; }
> + void setMSPropertyDecl(MSPropertyDecl *decl) { Decl = decl; }
> + bool isArrow() const { return IsArrow; }
> + void setArrow(bool isArrow) { IsArrow = isArrow; }
> + SourceLocation getMemberLoc() const { return MemberLoc; }
> + void setMemberLoc(SourceLocation memberLoc) { MemberLoc = memberLoc; }
> + DeclarationNameLoc getMemberDNLoc() const { return MemberDNLoc; }
> + void setMemberDNLoc(DeclarationNameLoc memberDNLoc) {
> + MemberDNLoc = memberDNLoc;
> + }
> + NestedNameSpecifierLoc getQualifierLoc() const { return QualifierLoc; }
> + void setQualifierLoc(NestedNameSpecifierLoc qualifierLoc) {
> + QualifierLoc = qualifierLoc;
> + }
>
> The statement/expression AST classes are generally meant to be immutable,
> and there's really no need to have any of these setters.
> Well, these setter's are for serialization. Every subclass of Expr has them...
> So they should be kept.
Many of them still have them, but we've come to prefer just befriending the
serialization classes over having tons of getters and setters.
> test/Parser/MicrosoftExtensions.c:
>
> Why is this in a C test case? Is this actually legal in C? I can't imagine
> it is, because it the entire language model relies on methods.
>
> You can parse the attribute in C, I guess, but you should always emit an
> error if you see it.
> On top of this test file, it says "-x objective-c++".
> So this file is treated as C++...
The much better way to handle that is by naming it the right way the first place. :)
> Explanation for all CheckPlaceholderExpr():
> 1. in Parse/ParseExpr.cpp, ParsePostfixExpressionSuffix()
> === Code Start ===
> class D
> {
> public:
> bool operator()() { return true; }
> };
>
> class C
> {
> public:
> __declspec(property(get=GetV)) D V;
> D GetV() { return D(); }
> };
>
> int main() {
> C c;
> bool b = c.V(); // If not added, clang will complain "c.V is not a function, nor function object"
> }
Ah. In Sema::ActOnCallExpr, please do this:
if (Fn->getType() == Context.PseudoObjectTy) {
ExprResult result = CheckPlaceholderExpr(*this, Fn);
if (result.isInvalid()) return ExprError();
Fn = result.take();
}
right after the isa<CXXPseudoDestructorExpr> block.
And please add this as a test case. :)
> 2. In Sema/SemaExpr.cpp, ActOnCallExpr():
> I admit that I'm not very thorough about this one...
> But here the CheckPlaceholderExpr() call does solve the follwing problem.
> === Code Start ===
> // If compiled with -ggdb, clang will crash and saying something about
> // "unexpected builtin-type" in some CG code.
>
> struct S2 {
> template <class T>
> void f(T t) {}
> };
>
> template <class T>
> struct S
> {
> __declspec(property(get=GetV)) int V;
> int GetV() { return 123; }
> void f() { S2 s2; s2.f(V); }
> };
>
> int main() {
> S<int> s;
> s.f();
> }
> === Code End ===
Ah, I think the problem here is that overload resolution and especially
template argument deduction aren't lowering out placeholder types. Please
hoist this loop out of the C++ block (for obscure reasons; just trust me)
and check:
hasPlaceholderType(BuiltinType::PseudoObject)
instead of checking for a particular expression kind.
And please add this as a test case. :)
> 3. In Sema/SemaExprCXX.cpp, BuildCXXNew()
> === Code Start ===
> template <class T>
> class C
> {
> public:
> __declspec(property(get=GetV)) T V;
> int GetV() { return 123; }
> void f() { int *p = new int[V]; } // If not added, clang will complain "V is dependent type, not integral type"
> };
>
> int main() {
> }
> === Code End ===
>
> 4. In Sema/SemaDecl.cpp, AddInitializerToDecl()
> === Code Start ===
> template <class T>
> class C
> {
> public:
> __declspec(property(get=GetV)) T V;
> T GetV() { return 123; }
> void f() { int t = V; } // If not added, clang will complain "V is dependent type"
> };
>
> int main() {
> C<int> c;
> c.f();
> }
> === Code End ===
Ah, good catch. Yes, this check is exactly right.
And please add this as a test case. :)
In general, this is looking very good, but please add a lot more tests.
You seem to have excellent test coverage; it would be good to
have some of that in-tree.
John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130415/a34ce0d0/attachment.html>
More information about the cfe-commits
mailing list