pekka.jaaskelainen requested changes to this revision.
pekka.jaaskelainen added a comment.
This revision now requires changes to proceed.
Only small issues found, plus it could use some more test cases.
================
Comment at: include/clang/AST/TypeLoc.h:2049
@@ +2048,3 @@
+ SourceRange getLocalSourceRange() const {
+ return SourceRange( getKWLoc() );
+ }
----------------
Extra white spaces.
================
Comment at: include/clang/Sema/DeclSpec.h:1425
@@ -1415,1 +1424,3 @@
+ struct PipeTypeInfo : TypeInfoCommon {
+ /// The access writes.
----------------
Indent error.
================
Comment at: include/clang/Sema/DeclSpec.h:1426
@@ +1425,3 @@
+ struct PipeTypeInfo : TypeInfoCommon {
+ /// The access writes.
+ unsigned AccessWrites : 3;
----------------
I think this needs a better comment than duplicating the variable name. What is
the purpose of the variable?
================
Comment at: include/clang/Sema/DeclSpec.h:1554
@@ +1553,3 @@
+ static DeclaratorChunk getPipe(unsigned TypeQuals,
+ SourceLocation Loc) {
+ DeclaratorChunk I;
----------------
Indent issue.
================
Comment at: include/clang/Serialization/ASTBitCodes.h:911
@@ +910,3 @@
+ TYPE_ADJUSTED = 42,
+ /// \brief An PipeType record.
+ TYPE_PIPE = 43
----------------
A
================
Comment at: lib/AST/ASTContext.cpp:3121
@@ +3120,3 @@
+/// getPipeType - Return pipe type for the specified type.
+QualType ASTContext::getPipeType(QualType T ) const {
+ // Unique pointers, to guarantee there is only one pointer of a particular
----------------
White space errror.
================
Comment at: lib/AST/ASTContext.cpp:3141
@@ +3140,3 @@
+ }
+ PipeType *New = new (*this, TypeAlignment) PipeType(T, Canonical);
+ Types.push_back(New);
----------------
Should we assign T to 'Canonical' if it already was Canonical or is this
intentional somehow?
================
Comment at: lib/AST/ItaniumMangle.cpp:2672
@@ +2671,3 @@
+ // <type> ::= U <source-name> <type> # vendor extended type qualifier
+ // (Until there's a standardized mangling...)
+ Out << "8ocl_pipe";
----------------
This seems to conform to SPIR 2.0, should we mention it here?
================
Comment at: lib/Parse/ParseDecl.cpp:4954
@@ +4953,3 @@
+
+ if ( D.getDeclSpec().isTypeSpecPipe() ) {
+ DeclSpec &DS = D.getMutableDeclSpec();
----------------
WS issues.
================
Comment at: lib/Parse/ParseDecl.cpp:4958
@@ +4957,3 @@
+ D.AddTypeInfo(DeclaratorChunk::getPipe(DS.getTypeQualifiers(),
+ DS.getPipeLoc()),
+ DS.getAttributes(),
----------------
Indent issues.
================
Comment at: lib/Sema/DeclSpec.cpp:729
@@ +728,3 @@
+
+ if ( TypeSpecType != TST_unspecified ) {
+ PrevSpec = DeclSpec::getSpecifierName((TST) TypeSpecType, Policy);
----------------
WS issues.
================
Comment at: lib/Sema/DeclSpec.cpp:735
@@ +734,3 @@
+
+ if ( isPipe ) {
+ TypeSpecPipe = TSP_pipe;
----------------
ditto
================
Comment at: lib/Sema/SemaType.cpp:1570
@@ -1564,3 +1569,3 @@
// attributes are pushed around.
- if (AttributeList *attrs = DS.getAttributes().getList())
- processTypeAttrs(state, Result, TAL_DeclSpec, attrs);
+ if ( !DS.isTypeSpecPipe() ) { // pipe attributes will be handled
later ( at GetFullTypeForDeclarator )
+ if ( AttributeList *attrs = DS.getAttributes().getList())
----------------
WS issues and an overlong line.
================
Comment at: lib/Sema/SemaType.cpp:1571
@@ +1570,3 @@
+ if ( !DS.isTypeSpecPipe() ) { // pipe attributes will be handled
later ( at GetFullTypeForDeclarator )
+ if ( AttributeList *attrs = DS.getAttributes().getList())
+ processTypeAttrs(state, Result, TAL_DeclSpec, attrs);
----------------
WS issues.
================
Comment at: lib/Sema/SemaType.cpp:1945
@@ +1944,3 @@
+QualType Sema::BuildPipeType(QualType T,
+ SourceLocation Loc) {
+ assert(!T->isObjCObjectType() && "Should build ObjCObjectPointerType");
----------------
Indent
================
Comment at: lib/Sema/TreeTransform.h:5254
@@ +5253,3 @@
+QualType TreeTransform<Derived>::TransformPipeType(TypeLocBuilder &TLB,
+ PipeTypeLoc TL) {
+ QualType ValueType = getDerived().TransformType(TLB, TL.getValueLoc());
----------------
Indent
================
Comment at: test/CodeGenOpenCL/pipe_types.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+
----------------
Can you add a couple of more checks? For example:
- the behavior when using CL1.2
- when the inner type of the pipe has a 'const' or similar qualifier
- It can only by a function arg. What if it's a local variable?
http://reviews.llvm.org/D14441
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
