[cfe-commits] Preliminary patch to support MSVC __declspec(property)
endlessroad1991 at gmail.com
endlessroad1991 at gmail.com
Mon Apr 15 19:27:06 PDT 2013
Rebased on r179580
+ bool isDeclspecPropertyAttribute() const {
> + return IsProperty && getKind() == AT_MsProperty && SyntaxUsed ==
> AS_Declspe
> + }
> +
>
> In what ways is this more than just IsProperty?
>
Nothing.. Just to make sure it's a __declspec(property)).
Removed last 2 conditions.
>
> + if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())
> + Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
> + diag::err_invalid_thread)
> + << DeclSpec::getSpecifierName(TSCS);
>
> Indentation.
>
Done
>
> + 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.
>
Done
>
> +
> + 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.
>
Done
>
> 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.
>
Done. Also did this for MSPropertyDecl.
>
> 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. :)
>
Done. Modified as you said.
>
> 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. :)
>
Done. Modified as you said.
>
> 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. :)
>
These 4 are added to testcases.
>
> 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.
>
I don't have tests; I only have ~100k lines of code to compile...
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...
However, I've add several testcases. See attached patch.
>
> John.
>
--
Best Regards, Tong Shen (沈彤)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130416/517ef8a8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1.patch
Type: application/octet-stream
Size: 62587 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130416/517ef8a8/attachment.obj>
More information about the cfe-commits
mailing list