<div dir="ltr">Sorry, forgot about the patch.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 15, 2013 at 10:35 PM, <a href="mailto:endlessroad1991@gmail.com">endlessroad1991@gmail.com</a> <span dir="ltr"><<a href="mailto:endlessroad1991@gmail.com" target="_blank">endlessroad1991@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Rebased on r179523<div class="gmail_extra"><br></div><div class="gmail_extra">Replay to John:<br><br><div class="gmail_quote">
<div class="im">On Tue, Apr 9, 2013 at 7:27 AM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On Apr 1, 2013, at 6:00 AM, <a href="mailto:endlessroad1991@gmail.com" target="_blank">endlessroad1991@gmail.com</a> wrote:<br>
> Rebased on r178459<br>
><br>
> All original tests still pass, and 2 new tests(one for Parser, one for Sema).<br>
<br>
</div>Major cross-cutting problem: none of the places where you're manually<br>
calling CheckPlaceholderExpr should be necessary. I'm positive that<br>
we're already filtering out placeholders in these places; if we weren't,<br>
we'd have a serious bug that would hit a ton of Objective-C code.<br>
<br>
The other nice thing about relying on the existing machinery for this is<br>
that you wouldn't have a ton of latent bugs about not looking through parens. :)<br></blockquote></div><div>Yes, I'm aware of that :-)</div><div>I will explain each case of CheckPlaceholderExpr() in the last.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
SemaTemplateInstantiateDecl.cpp:<br>
<div><br>
+ if (DI->getType()->isInstantiationDependentType() ||<br>
+ DI->getType()->isVariablyModifiedType()) {<br>
<br>
</div>Why are you checking for variably-modified types here?<br></blockquote></div><div>I just copied most of handleField() into handleMSProperty(),</div><div>In the attached new patch, it will complain about VariablyModifiedType</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ if (CXXRecordDecl *Parent =<br>
+ dyn_cast<CXXRecordDecl>(Property->getDeclContext())) {<br>
<div>+ if (Parent->isAnonymousStructOrUnion() &&<br>
+ Parent->getRedeclContext()->isFunctionOrMethod())<br>
+ SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, Property);<br>
+ }<br>
<br>
</div>There should be an invariant that the parent of an MSPropertyDecl is<br>
a CXXRecordDecl.<br></blockquote></div><div>Done. This "if" is removed.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
SemaDecl.cpp:<br>
<br>
This is all C++-specific and should go in SemaDeclCXX.cpp.<br></blockquote></div><div>Done. Moved to SemaDeclCXX.cpp</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ DiagnoseFunctionSpecifiers(D.getDeclSpec());<br>
<div>+<br>
+ if (D.getDeclSpec().isThreadSpecified())<br>
+ Diag(D.getDeclSpec().getThreadSpecLoc(), diag::err_invalid_thread);<br>
+ if (D.getDeclSpec().isConstexprSpecified())<br>
+ Diag(D.getDeclSpec().getConstexprSpecLoc(), diag::err_invalid_constexpr)<br>
+ << 2;<br>
<br>
</div>You should also complain about the presence of a storage class specifier.<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ MSPropertyDecl *NewFD;<br>
<br>
Style nit: "NewPD" instead of "NewFD".<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><br>
+ llvm::StringRef GetterName = MSPropertyAttr->getScopeName() ?<br>
+ MSPropertyAttr->getScopeName()->getName() : StringRef();<br>
+ llvm::StringRef SetterName = MSPropertyAttr->getParameterName() ?<br>
+ MSPropertyAttr->getParameterName()->getName() : StringRef();<br>
<br>
</div>Why are you storing getter/setter names as a StringRefs instead of an<br>
IdentifierInfo*? Identifiers are (1) more compact, (2) better supported by<br>
the serialization libraries, and (3) required by lookup anyway.<br></blockquote></div><div>Done. And this does make the code much cleaner! I should have realized it before.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
SemaPseudoObject.cpp:<br>
<div><br>
+ } else if (MSPropertyRefExpr *refExpr<br>
+ = dyn_cast<MSPropertyRefExpr>(opaqueRef)) {<br>
+ return refExpr;<br>
<br>
</div>You need to implement this correctly; you should follow the lead<br>
of the existing implementations.<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
SemaDeclCXX.cpp:<br>
<br>
+static AttributeList *getMSPropertyAttr(AttributeList *list) {<br>
+ for (AttributeList* it = list; it != 0; it = it->getNext())<br>
<div>+ if (it->getName()->getName() == "property")<br>
<br>
</div>if (it->getKind() == AttributeList::AT_property)<br>
<br>
I'm a little bothered by doing this here instead of filtering it out beforehand,<br>
but it's probably okay; attributes on fields are relatively uncommon.<br></blockquote></div><div>I don't like this either, but ParsedAttr are only attached to FieldDecl after it's created...</div><div>
And here, we need to judge whether we should create FieldDecl or MSPropertyDecl.</div><div>So I just put it here.</div><div>Any ideas?</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
AttributeList.cpp:<br>
<br>
- if (ScopeName)<br>
- Buf += ScopeName->getName();<br>
- // Ensure that in the case of C++11 attributes, we look for '::foo' if it is<br>
- // unscoped.<br>
- if (ScopeName || SyntaxUsed == AS_CXX11)<br>
- Buf += "::";<br>
+ // For property, "ScopeName" is used to store getter function name<br>
+ if (AttrName != "property") {<br>
+ if (ScopeName)<br>
+ Buf += ScopeName->getName();<br>
+ // Ensure that in the case of C++11 attributes, we look for '::foo' if it is<br>
+ // unscoped.<br>
+ if (ScopeName || SyntaxUsed == AS_CXX11)<br>
+ Buf += "::";<br>
+ }<br>
<br>
Okay, this is actually a hot enough path that we'd really rather not introduce<br>
a string compare into it. Figure out some other way to store the second<br>
string; you could throw things like MessageExpr into a union.<br></blockquote></div><div>Done. Turns out that this problem has been encountered before: availability and typetag attributes.</div><div>I used the same mechanism as existing code: malloc more than sizeof(AttributeList), and use the extra space.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
ASTBitCodes.h:<br>
<div><br>
+ /// \brief A __delcpec(property) record.<br>
+ DECL_MS_PROPERTY,<br>
<br>
</div>By prevailing style in this file, this should be "An MSPropertyDecl record."<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
ExprCXX.h:<br>
<br>
+ SourceLocation MemberLoc;<br>
+ DeclarationNameLoc MemberDNLoc;<br>
+ bool IsArrow;<br>
<br>
Please put the bool immediately after the SourceLocation; it'll pack better<br>
on 64-bit architectures.<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ SourceLocation getLocStart() const {<br>
+ if (QualifierLoc) {<br>
+ return QualifierLoc.getBeginLoc();<br>
+ } else {<br>
+ SourceLocation result = BaseExpr->getLocStart();<br>
+ if (result.isValid())<br>
+ return result;<br>
+ else<br>
+ return MemberLoc;<br>
+ }<br>
+ }<br>
<br>
You've got this mixed up. The start loc is the start of the base<br>
expression, unless that's an implicit this access, in which case it's the<br>
start of the qualifier, unless that doesn't exist, in which case it's the<br>
member loc.<br>
<br>
You should have an isImplicitAccess() method like MemberExpr does.<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><br>
+ Expr* getBaseExpr() const { return BaseExpr; }<br>
+ void setBaseExpr(Expr *baseExpr) { BaseExpr = baseExpr; }<br>
+ MSPropertyDecl* getMSPropertyDecl() const { return Decl; }<br>
+ void setMSPropertyDecl(MSPropertyDecl *decl) { Decl = decl; }<br>
+ bool isArrow() const { return IsArrow; }<br>
+ void setArrow(bool isArrow) { IsArrow = isArrow; }<br>
+ SourceLocation getMemberLoc() const { return MemberLoc; }<br>
+ void setMemberLoc(SourceLocation memberLoc) { MemberLoc = memberLoc; }<br>
</div>+ DeclarationNameLoc getMemberDNLoc() const { return MemberDNLoc; }<br>
+ void setMemberDNLoc(DeclarationNameLoc memberDNLoc) {<br>
+ MemberDNLoc = memberDNLoc;<br>
+ }<br>
+ NestedNameSpecifierLoc getQualifierLoc() const { return QualifierLoc; }<br>
+ void setQualifierLoc(NestedNameSpecifierLoc qualifierLoc) {<br>
+ QualifierLoc = qualifierLoc;<br>
+ }<br>
<br>
The statement/expression AST classes are generally meant to be immutable,<br>
and there's really no need to have any of these setters.<br></blockquote></div><div>Well, these setter's are for serialization. Every subclass of Expr has them...</div><div>So they should be kept.</div><div class="im">
<div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Attr.td:<br>
<br>
+def MsProperty : InheritableAttr {<br>
<div>+ let Spellings = [Declspec<"property">];<br>
+}<br>
</div>+<br>
<br>
It's not actually Inheritable; properties can't be redeclared.<br></blockquote></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
test/Parser/MicrosoftExtensions.c:<br>
<br>
Why is this in a C test case? Is this actually legal in C? I can't imagine<br>
it is, because it the entire language model relies on methods.<br>
<br>
You can parse the attribute in C, I guess, but you should always emit an<br>
error if you see it.<br></blockquote></div><div>On top of this test file, it says "-x objective-c++".</div><div>So this file is treated as C++...</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+struct S8 {<br>
+ __declspec(property) int V0; /* expected-error {{expected '(' after 'property'}} */<br>
+ __declspec(property()) int V1; /* expected-error {{expected 'get' or 'put' in __declspec(property)}} expected-error {{no getter or putter in __declspec(property)}} */<br>
+ __declspec(property(set)) int V2; /* expected-error {{expected 'get' or 'put' in __declspec(property)}} expected-error {{no getter or putter in __declspec(property)}} */<br>
+ __declspec(property(get)) int V3; /* expected-error {{expected '=' in __declspec(property)}} expected-error {{no getter or putter in __declspec(property)}} */<br>
+ __declspec(property(get&)) int V4; /* expected-error {{expected '=' in __declspec(property)}} expected-error {{no getter or putter in __declspec(property)}} */<br>
+ __declspec(property(get=)) int V5; /* expected-error {{expected identifier in __declspec(property)}} expected-error {{no getter or putter in __declspec(property)}} */<br>
+ __declspec(property(get=GetV)) int V6; /* no-warning */<br>
+ __declspec(property(get=GetV=)) int V7; /* expected-error {{expected ',' or ')' in __declspec(property)}} */<br>
+ __declspec(property(get=GetV,)) int V8; /* expected-error {{expected 'get' or 'put' in __declspec(property)}} */<br>
+ __declspec(property(get=GetV,put=SetV)) int V9; /* no-warning */<br>
+ __declspec(property(get=GetV,put=SetV,get=GetV)) int V10; /* expected-error {{duplicate accessor in __declspec(property)}} */<br>
+};<br>
<br>
You do not have a single test case that verifies that you can actually<br>
access one of these.<br></blockquote></div><div>Added.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888"><br>
John.<br>
</font></span></blockquote></div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div>Explanation for all CheckPlaceholderExpr():</div><div class="gmail_extra">1. in Parse/ParseExpr.cpp, ParsePostfixExpressionSuffix()</div>
<div class="gmail_extra">=== Code Start ===</div><div class="gmail_extra"><div class="gmail_extra">class D</div><div class="gmail_extra">{</div><div class="gmail_extra">public:</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>bool operator()() { return true; }</div>
<div class="gmail_extra">};</div><div class="gmail_extra"><br></div><div class="gmail_extra">class C</div><div class="gmail_extra">{</div><div class="gmail_extra">public:</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>__declspec(property(get=GetV)) D V;</div>
<div class="gmail_extra"><span style="white-space:pre-wrap"> </span>D GetV() { return D(); }</div><div class="gmail_extra">};</div><div class="gmail_extra"><br></div><div class="gmail_extra">int main() {</div><div class="gmail_extra">
<span style="white-space:pre-wrap"> </span>C c;</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>bool b = c.V(); // If not added, clang will complain "c.V is not a function, nor function object"</div>
<div class="gmail_extra">}</div><div class="gmail_extra">=== Code End ===</div><div class="gmail_extra"><br></div><div>2. In Sema/SemaExpr.cpp, ActOnCallExpr():</div><div>I admit that I'm not very thorough about this one...</div>
<div>But here the CheckPlaceholderExpr() call does solve the follwing problem.</div><div>=== Code Start ===</div><div><div>// If compiled with -ggdb, clang will crash and saying something about</div><div>
// "unexpected builtin-type" in some CG code.</div><div><br></div><div>struct S2 {</div><div><span style="white-space:pre-wrap"> </span>template <class T></div><div><span style="white-space:pre-wrap"> </span>void f(T t) {}</div>
<div>};</div><div><br></div><div>template <class T></div><div>struct S</div><div>{</div><div><span style="white-space:pre-wrap"> </span>__declspec(property(get=GetV)) int V;</div><div><span style="white-space:pre-wrap"> </span>int GetV() { return 123; }</div>
<div><span style="white-space:pre-wrap"> </span>void f() { S2 s2; s2.f(V); }</div><div>};</div><div><br></div><div>int main() {</div><div><span style="white-space:pre-wrap"> </span>S<int> s;</div><div><span style="white-space:pre-wrap"> </span>s.f();</div>
<div>}</div></div><div>=== Code End ===</div></div><div class="gmail_extra"><br></div><div class="gmail_extra">3. In Sema/SemaExprCXX.cpp, BuildCXXNew()</div><div class="gmail_extra">=== Code Start ===</div>
<div class="gmail_extra"><div class="gmail_extra">template <class T></div><div class="gmail_extra">class C</div><div class="gmail_extra">{</div><div class="gmail_extra">public:</div><div class="gmail_extra"> __declspec(property(get=GetV)) T V;</div>
<div class="gmail_extra"> int GetV() { return 123; }</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>void f() { int *p = new int[V]; } // If not added, clang will complain "V is dependent type, not integral type"</div>
<div class="gmail_extra">};</div><div class="gmail_extra"><br></div><div class="gmail_extra">int main() {</div><div class="gmail_extra">}</div></div><div class="gmail_extra">=== Code End ===</div><div class="gmail_extra">
<br></div><div class="gmail_extra">4. In Sema/SemaDecl.cpp, AddInitializerToDecl()</div><div class="gmail_extra">=== Code Start ===</div><div class="gmail_extra"><div class="gmail_extra">template <class T></div>
<div class="gmail_extra">class C</div><div class="gmail_extra">{</div><div class="gmail_extra">public:</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>__declspec(property(get=GetV)) T V;</div>
<div class="gmail_extra"><span style="white-space:pre-wrap"> </span>T GetV() { return 123; }</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>void f() { int t = V; } // If not added, clang will complain "V is dependent type"</div>
<div class="gmail_extra">};</div><div class="gmail_extra"><br></div><div class="gmail_extra">int main() {</div><div class="gmail_extra"><span style="white-space:pre-wrap"> </span>C<int> c;</div><div class="gmail_extra">
<span style="white-space:pre-wrap"> </span>c.f();</div><div class="gmail_extra">}</div></div><div class="gmail_extra">=== Code End ===</div><div class="im"><div class="gmail_extra"><div><br></div>-- <br>Best Regards, Tong Shen (沈彤)
</div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Best Regards, Tong Shen (沈彤)
</div>