<div dir="ltr">Rebased on r179580<div class="gmail_extra"><br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div>+ bool isDeclspecPropertyAttribute() const {</div><div>+ return IsProperty && getKind() == AT_MsProperty && SyntaxUsed == AS_Declspe</div><div>+ }</div><div>+</div><div><br></div><div>In what ways is this more than just IsProperty?</div>
</div></div></blockquote><div style>Nothing.. Just to make sure it's a __declspec(property)).</div><div style>Removed last 2 conditions.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><br></div><div><div>+ if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())</div><div>+ Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),</div><div>+ diag::err_invalid_thread)</div>
<div>+ << DeclSpec::getSpecifierName(TSCS);</div><div><br></div></div><div>Indentation.</div></div></div></blockquote><div style>Done</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="im"><div><br></div><div><div>+ if (II) Loc = D.getIdentifierLoc();</div></div><div><br></div></div><div>What's with checks like these? When can you make an anonymous property</div>
<div>declaration? I think you should just be checking this early and then bailing out</div><div>if you don't have a name.</div></div></div></blockquote><div style>Done</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><br></div><div><div>+ </div><div>+ case Expr::MSPropertyRefExprClass:</div><div>+ return CT_Can;</div></div><div><br></div><div>This case should assert with llvm_unreachable. If we're processing a bare</div>
<div>MSPropertyRefExpr for whether it can throw, we've done something wrong.</div></div></div></blockquote><div style>Done</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="im"><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">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>Yes, I'm aware of that :-)</div><div>I will explain each case of CheckPlaceholderExpr() in the last.</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">
<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>I just copied most of handleField() into handleMSProperty(),</div><div>In the attached new patch, it will complain about VariablyModifiedType</div>
</div></div></div></blockquote><div><br></div></div>Ah, sure, you definitely do need to detect this.</div><div><br></div><div><div class="im"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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">
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>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></div></div></blockquote><div><br></div></div>I think it's fine.</div>
<div> </div><div><div class="im"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
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>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></div></div></blockquote><div><br></div></div>Sure, good call.</div><div><div class="im"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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>Well, these setter's are for serialization. Every subclass of Expr has them...</div><div>So they should be kept.</div></div></div></div>
</blockquote><div><br></div></div>Many of them still have them, but we've come to prefer just befriending the</div><div>serialization classes over having tons of getters and setters.</div></div></blockquote><div style>
Done. Also did this for MSPropertyDecl.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div><div class="im">
<blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
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>On top of this test file, it says "-x objective-c++".</div><div>So this file is treated as C++...</div></div></div></div></blockquote><div><br></div></div><div>The much better way to handle that is by naming it the right way the first place. :)</div>
<div class="im"><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">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></div></blockquote><div><br></div></div><div>Ah. In Sema::ActOnCallExpr, please do this:</div><div> if (Fn->getType() == Context.PseudoObjectTy) {</div><div><div> ExprResult result = CheckPlaceholderExpr(*this, Fn);</div>
<div> if (result.isInvalid()) return ExprError();</div><div> Fn = result.take();</div><div> }</div><div>right after the isa<CXXPseudoDestructorExpr> block.</div><div><br></div><div>And please add this as a test case. :)</div>
</div></div></div></blockquote><div style>Done. Modified as you said.</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div><div><br></div></div><div class="im"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">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></blockquote><div><br></div></div>Ah, I think the problem here is that overload resolution and especially</div><div>template argument deduction aren't lowering out placeholder types. Please</div>
<div>hoist this loop out of the C++ block (for obscure reasons; just trust me)</div><div>and check:</div><div> hasPlaceholderType(BuiltinType::PseudoObject)</div><div>instead of checking for a particular expression kind.</div>
<div><br></div><div>And please add this as a test case. :)</div></div></blockquote><div style>Done. Modified as you said.</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div class="im"><div><br></div><div><blockquote type="cite"><div dir="ltr"><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></blockquote><br></div></div><div>Ah, good catch. Yes, this check is exactly right.</div>
<br><div>And please add this as a test case. :)</div></div></blockquote><div style>These 4 are added to testcases.</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div>In general, this is looking very good, but please add a lot more tests.</div><div>You seem to have excellent test coverage; it would be good to</div><div>have some of that in-tree.</div>
</div></blockquote><div style>I don't have tests; I only have ~100k lines of code to compile...</div><div style>A project originally developed by MSVC needed to be ported to linux, and I was assigned this task. Property is used extensively all around the code, which made it practically impossible to rewrite those properties one by one. So I came up with this modify-compiler plan...</div>
<div style>However, I've add several testcases. See attached patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<span class="HOEnZb"><font color="#888888"><div><br></div><div>John.</div></font></span></div></blockquote></div><br><br clear="all"><div><br></div>-- <br>Best Regards, Tong Shen (沈彤)
</div></div>