<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Apr 15, 2013, at 7:35 AM, <a href="mailto:endlessroad1991@gmail.com">endlessroad1991@gmail.com</a> wrote:</div><blockquote type="cite"><div dir="ltr">Rebased on r179523</div></blockquote><div><br></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><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><br></div><div><div>+ if (II) Loc = D.getIdentifierLoc();</div></div><div><br></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><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><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; position: static; z-index: auto; "><div class="im">On Apr 1, 2013, at 6:00 AM, <a href="mailto:endlessroad1991@gmail.com">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 style="">Yes, I'm aware of that :-)</div><div style="">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; position: static; z-index: auto; ">
<br>
SemaTemplateInstantiateDecl.cpp:<br>
<div class="im"><br>
+ if (DI->getType()->isInstantiationDependentType() ||<br>
+ DI->getType()->isVariablyModifiedType()) {<br>
<br>
</div>Why are you checking for variably-modified types here?<br></blockquote><div style="">I just copied most of handleField() into handleMSProperty(),</div><div style="">In the attached new patch, it will complain about VariablyModifiedType</div></div></div></div></blockquote><div><br></div>Ah, sure, you definitely do need to detect this.</div><div><br></div><div><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; position: static; z-index: auto; ">
SemaDeclCXX.cpp:<br>
<br>
+static AttributeList *getMSPropertyAttr(AttributeList *list) {<br>
+ for (AttributeList* it = list; it != 0; it = it->getNext())<br>
<div class="im">+ 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 style="">I don't like this either, but ParsedAttr are only attached to FieldDecl after it's created...</div><div style="">
And here, we need to judge whether we should create FieldDecl or MSPropertyDecl.</div><div style="">So I just put it here.</div><div style="">Any ideas?</div></div></div></div></blockquote><div><br></div>I think it's fine.</div><div> </div><div><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; position: static; z-index: auto; ">
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 style="">Done. Turns out that this problem has been encountered before: availability and typetag attributes.</div><div style="">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>Sure, good call.</div><div><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 class="im"><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 style="">Well, these setter's are for serialization. Every subclass of Expr has them...</div><div style="">So they should be kept.</div></div></div></div></blockquote><div><br></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><br></div><div><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; position: static; z-index: auto; ">
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 style="">On top of this test file, it says "-x objective-c++".</div><div style="">So this file is treated as C++...</div></div></div></div></blockquote><div><br></div><div>The much better way to handle that is by naming it the right way the first place. :)</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">Explanation for all CheckPlaceholderExpr():</div><div class="gmail_extra" style="">1. in Parse/ParseExpr.cpp, ParsePostfixExpressionSuffix()</div>
<div class="gmail_extra" style="">=== Code Start ===</div><div class="gmail_extra" style=""><div class="gmail_extra">class D</div><div class="gmail_extra">{</div><div class="gmail_extra">public:</div><div class="gmail_extra"><span class="" style="white-space:pre"> </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 class="" style="white-space:pre"> </span>__declspec(property(get=GetV)) D V;</div>
<div class="gmail_extra"><span class="" style="white-space:pre"> </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 class="" style="white-space:pre"> </span>C c;</div><div class="gmail_extra"><span class="" style="white-space:pre"> </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>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><br></div></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra" style=""><div class="gmail_extra" style="">2. In Sema/SemaExpr.cpp, ActOnCallExpr():</div><div style="">I admit that I'm not very thorough about this one...</div>
<div style="">But here the CheckPlaceholderExpr() call does solve the follwing problem.</div><div style="">=== Code Start ===</div><div style=""><div>// If compiled with -ggdb, clang will crash and saying something about</div><div style="">
// "unexpected builtin-type" in some CG code.</div><div><br></div><div>struct S2 {</div><div><span class="" style="white-space:pre"> </span>template <class T></div><div><span class="" style="white-space:pre"> </span>void f(T t) {}</div>
<div>};</div><div><br></div><div>template <class T></div><div>struct S</div><div>{</div><div><span class="" style="white-space:pre"> </span>__declspec(property(get=GetV)) int V;</div><div><span class="" style="white-space:pre"> </span>int GetV() { return 123; }</div>
<div><span class="" style="white-space:pre"> </span>void f() { S2 s2; s2.f(V); }</div><div>};</div><div><br></div><div>int main() {</div><div><span class="" style="white-space:pre"> </span>S<int> s;</div><div><span class="" style="white-space:pre"> </span>s.f();</div>
<div>}</div></div><div style="">=== Code End ===</div></div></div></blockquote><div><br></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><br></div><div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra" style="">3. In Sema/SemaExprCXX.cpp, BuildCXXNew()</div><div class="gmail_extra" style="">=== Code Start ===</div>
<div class="gmail_extra" style=""><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 class="" style="white-space:pre"> </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" style="">=== Code End ===</div><div class="gmail_extra" style="">
<br></div><div class="gmail_extra" style="">4. In Sema/SemaDecl.cpp, AddInitializerToDecl()</div><div class="gmail_extra" style="">=== Code Start ===</div><div class="gmail_extra" style=""><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 class="" style="white-space:pre"> </span>__declspec(property(get=GetV)) T V;</div>
<div class="gmail_extra"><span class="" style="white-space:pre"> </span>T GetV() { return 123; }</div><div class="gmail_extra"><span class="" style="white-space:pre"> </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 class="" style="white-space:pre"> </span>C<int> c;</div><div class="gmail_extra">
<span class="" style="white-space:pre"> </span>c.f();</div><div class="gmail_extra">}</div></div><div class="gmail_extra" style="">=== Code End ===</div></div></blockquote><br></div><div>Ah, good catch. Yes, this check is exactly right.</div><br><div>And please add this as a test case. :)</div><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><br></div><div>John.</div></body></html>