rjmccall added inline comments.
================
Comment at: include/clang/AST/Decl.h:977
+
+ unsigned EscapingByref : 1;
};
----------------
This doesn't actually seem to be initialized anywhere.
================
Comment at: include/clang/AST/Decl.h:1422
+
+ bool isNonEscapingByref() const;
+
----------------
Documentation comments? I'm not opposed to using `Byref` as a shorthand here,
but please remember that a lot of Clang maintainers aren't familiar with the
blocks extension, and those that are aren't necessarily familiar with the
non-escaping optimization.
================
Comment at: include/clang/AST/Decl.h:3888
+ bool isNonEscapingByref() const {
+ return VariableAndFlags.getPointer()->isNonEscapingByref();
+ }
----------------
Please call `getVariable()` here for clarity.
================
Comment at: include/clang/Sema/ScopeInfo.h:438
+ EscapingBlocks.erase(BD);
+ }
+
----------------
Please describe the expected operation in comments here: that blocks are added
by default and then removed when the expression is provably used in a
non-escaping way.
================
Comment at: lib/CodeGen/CGBlocks.cpp:497
+ return VD->isNonEscapingByref() ?
+ CGF.getContext().getLValueReferenceType(VD->getType()) :
VD->getType();
}
----------------
Do you need to worry about the variable already having a reference type?
================
Comment at: lib/CodeGen/CGBlocks.cpp:1310
- if (capture.fieldType()->isReferenceType())
+ if (capture.fieldType()->isReferenceType() || variable->isNonEscapingByref())
addr = EmitLoadOfReference(MakeAddrLValue(addr, capture.fieldType()));
----------------
Isn't the field type already a reference type in this case?
You should leave a comment that you're relying on that, of course.
================
Comment at: lib/Sema/Sema.cpp:1410
+ VD->setEscapingByref();
+ }
+
----------------
It's okay to walk over an unordered set here because you're just changing
memory. However, you don't actually need it to be a set; you can just remember
all the blocks in the function and then ignore them if they're marked as
non-escaping.
================
Comment at: lib/Sema/Sema.cpp:1412
+
+ for (VarDecl *VD : FSI.ByrefBlockVars)
+ // __block variables might require us to capture a copy-initializer.
----------------
In contrast, it's not okay to walk over an unordered set here because the loop
body can emit diagnostics and/or instantiate code.
Fortunately, you don't actually need `ByrefBlockVars` to be a set at all; you
can just use an `llvm::TinyPtrVector'.
================
Comment at: lib/Sema/Sema.cpp:1414
+ // __block variables might require us to capture a copy-initializer.
+ if (VD->isEscapingByref()) {
+ QualType T = VD->getType();
----------------
Please use an early `continue` so you can de-indent the rest of the loop body.
================
Comment at: lib/Sema/Sema.cpp:1420
+ // constructing this copy.
+ if (T->isStructureOrClassType()) {
+ EnterExpressionEvaluationContext scope(
----------------
Should the contents of this block be in a helper function?
================
Comment at: lib/Sema/Sema.cpp:1451
+ // Call this function before popping the current function scope.
+ markEscapingByrefs(*FunctionScopes.back(), *this);
+
----------------
Why before?
Repository:
rC Clang
https://reviews.llvm.org/D51564
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
