aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/Attr.td:1329
StringArgument<"ISA", 1>];
let Documentation = [AMDGPUFlatWorkGroupSizeDocs];
let Subjects = SubjectList<[Function], ErrorDiag, "ExpectedKernelFunction">;
----------------
Please ensure that the attribute documentation properly reflects the changes
you've made here. At the very least, I would expect the docs to mention that
these can now be in dependent contexts.
The same applies for the other attributes.
================
Comment at: include/clang/Basic/Attr.td:1368
let Subjects = SubjectList<[Function], ErrorDiag>;
let Documentation = [Undocumented];
+ let TemplateDependent = 1;
----------------
Since you're changing the definition of the attribute, it would also be nice to
add some documentation to AttrDocs.td for this.
================
Comment at: lib/CodeGen/TargetInfo.cpp:7665-7666
+namespace
+{
+ inline
----------------
This should be a static function rather than in an inline namespace.
================
Comment at: lib/CodeGen/TargetInfo.cpp:7671
+ llvm::APSInt r{32, 0};
+ if (E) E->EvaluateAsInt(r, Ctx);
+
----------------
If there is no expression given, why should this return an `APSInt` for 0? This
seems more like something you would assert.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5583-5584
+namespace
+{
+ inline
----------------
static function instead, please.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5586
+ inline
+ bool checkAllAreIntegral(const AttributeList &Attr, Sema &S) {
+ for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
----------------
We usually put the `Sema` object first in the parameter list in this file,
don't we?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5587-5588
+ bool checkAllAreIntegral(const AttributeList &Attr, Sema &S) {
+ for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+ auto e = Attr.getArgAsExpr(i);
+ if (e && !e->getType()->isIntegralOrEnumerationType()) {
----------------
These variables should not be lower-case per our usual coding conventions.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5588
+ for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+ auto e = Attr.getArgAsExpr(i);
+ if (e && !e->getType()->isIntegralOrEnumerationType()) {
----------------
Please spell the type out explicitly.
https://reviews.llvm.org/D39857
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
