aaron.ballman added a comment.
Thank you for your patience while I sat and thought about this for a while. I'm
not against the idea, but I've definitely got some design concerns with it
which I've pointed out in the review. I think this also needs considerably more
testing of the codegen and semantic behaviors around improper use of `this`.
It's also worth investigating what else needs modifications to support this
properly -- I would imagine the static analyzer cares about CXXThisExpr
semantics, and I'm betting thread safety analysis does as well. If you haven't
already, you should search for uses of `CXXThisExpr` in the code base to spot
other areas that need test coverage to prove they still work properly in HLSL.
================
Comment at: clang/include/clang/AST/ExprCXX.h:1158-1161
+ static CXXThisExpr *Create(const ASTContext &C, SourceLocation Loc,
+ QualType Ty, bool IsImplicit) {
+ return new (C) CXXThisExpr(Loc, Ty, IsImplicit);
+ }
----------------
I don't think we need to add the `Create()` method -- but if there's a reason
you really want to add it, I think we should protect the constructor so it's
clear which interface callers should use. I think that sort of change is
logically orthogonal to the rest of the patch and would be better as a separate
(NFC) change.
================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:179
+ AST, SourceLocation(),
+ Constructor->getThisType().getTypePtr()->getPointeeType(), true);
+ This->setValueKind(ExprValueKind::VK_LValue);
----------------
python3kgae wrote:
> gracejennings wrote:
> > python3kgae wrote:
> > > gracejennings wrote:
> > > > python3kgae wrote:
> > > > > gracejennings wrote:
> > > > > > python3kgae wrote:
> > > > > > > Should this be a reference type?
> > > > > > Could you expand on the question? I'm not sure I understand what
> > > > > > you're asking. The two changes in this file were made to update the
> > > > > > previous RWBuffer implementation
> > > > > The current code will create CXXThisExpr with the pointeeType.
> > > > > I thought it should be a reference type of the pointeeType.
> > > > >
> > > > > Like in the test,
> > > > > CXXThisExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>>
> > > > > 'RWBuffer<element_type> *' implicit this
> > > > >
> > > > > The type is RWBuffer<element_type> * before,
> > > > > I expected this patch will change it to
> > > > > RWBuffer<element_type> &.
> > > > The change that makes it more reference like than c++ from:
> > > >
> > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:17> 'int' lvalue ->First
> > > > 0x{{[0-9A-Fa-f]+}}`
> > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} <col:11> 'Pair *' this`
> > > >
> > > > to hlsl with this change
> > > >
> > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:16> 'int' lvalue .First
> > > > 0x{{[0-9A-Fa-f]+}}`
> > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} <col:11> 'Pair' lvalue this`
> > > I guess we have to change clang codeGen for this anyway.
> > >
> > > Not sure which has less impact for codeGen side, lvalue like what is in
> > > the current patch or make it a lvalue reference? My feeling is lvalue
> > > reference might be eaiser.
> > >
> > > Did you test what needs to change for clang codeGen on top of the current
> > > patch?
> > >
> > With just the lvalue change in the current patch there should be no
> > additional changes needed in clang CodeGen on top of the current patch
> Since we already get the codeGen working with this patch,
> it would be nice to have a codeGen test.
I think it's an interesting question to consider, but I have some concerns.
Consider code like this:
```
struct S {
int Val = 0;
void foo() {
Val = 10;
S Another;
this = Another; // If this is a non-const reference, we can assign into
it...
print(); // ... so do we print 0 or 10?
// When this function ends, is `this` destroyed because `Another` goes out
of scope?
}
void print() {
std::cout << Val;
}
};
```
I think we need to prevent code like that from working. But it's not just
direct assignments that are a concern. Consider:
```
struct S;
void func(S &Out, S &In) {
Out = In;
}
struct S {
int Val = 0;
void foo() {
Val = 10;
S Another;
func(this, Another); // Same problem here!
print();
}
void print() {
std::cout << Val;
}
};
```
Another situation that I'm not certain of is whether HLSL supports
tail-allocation where the code is doing something like `this + 1` to get to the
start of the trailing objects.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
