[PATCH] OpenMP threadprivate directive
Dmitri Gribenko
gribozavr at gmail.com
Sat Feb 16 07:18:13 PST 2013
This is mostly a style review; Doug should do a real review.
================
Comment at: include/clang/AST/DeclOpenMP.h:23-24
@@ +22,4 @@
+
+/// OMPThreadPrivateDecl - This represents #pragma omp threadprivate ...
+/// directive
+class OMPThreadPrivateDecl : public Decl {
----------------
When adding new code, please don't duplicate function or class name name in the comment.
================
Comment at: include/clang/AST/DeclOpenMP.h:26
@@ +25,3 @@
+class OMPThreadPrivateDecl : public Decl {
+ size_t NumVars;
+
----------------
While I agree that `size_t` is the correct type here, Clang uses `unsigned` to hold the number of various things that occur in the source code. 2^32+1 is well over all Clang implementation limits, so everything should fit into `unsigned`.
(Also parameter types in member functions below.)
================
Comment at: include/clang/AST/DeclOpenMP.h:33-34
@@ +32,4 @@
+
+ VarDecl const * const *getVars() const {
+ return reinterpret_cast<VarDecl const * const *>(this + 1);
+ }
----------------
`const VarDecl` is more in line with LLVM style.
================
Comment at: include/clang/AST/DeclOpenMP.h:49
@@ +48,3 @@
+ typedef VarDecl **varlist_iterator;
+ typedef VarDecl const * const *varlist_const_iterator;
+
----------------
`const VarDecl`
================
Comment at: include/clang/AST/DeclOpenMP.h:58
@@ +57,3 @@
+
+ void setVars(VarDecl **B);
+
----------------
Please make this private and add `friend class ASTDeclReader`.
Also, what does parameter name `B` stand for? The definition of this function uses `VL`.
================
Comment at: include/clang/AST/DeclVisitor.h:21
@@ -20,2 +20,3 @@
#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclOpenMP.h"
----------------
Please sort includes alphabetically.
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:22
@@ -21,2 +21,3 @@
#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclOpenMP.h"
#include "clang/AST/Expr.h"
----------------
Please sort includes alphabetically.
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1392-1393
@@ +1391,4 @@
+DEF_TRAVERSE_DECL(OMPThreadPrivateDecl, {
+ for (OMPThreadPrivateDecl::varlist_iterator P = D->varlist_begin(),
+ E = D->varlist_end();
+ P != E; ++P) {
----------------
Align `P` and `E`. Also, `I` is more in line with LLVM style.
Same for `tools/libclang/RecursiveASTVisitor.h`.
================
Comment at: include/clang/Parse/Parser.h:19
@@ -18,2 +18,3 @@
#include "clang/Basic/Specifiers.h"
+#include "clang/Basic/OpenMPKinds.h"
#include "clang/Lex/CodeCompletionHandler.h"
----------------
Please sort includes alphabetically.
================
Comment at: include/clang/Parse/Parser.h:2099-2101
@@ -2095,1 +2098,5 @@
+ // OpenMP: Directives and clauses
+ DeclGroupPtrTy ParseOpenMPDeclarativeDirective();
+ bool ParseOpenMPSimpleVarList(OpenMPDirectiveKind Kind,
+ SmallVector<DeclarationNameInfo, 5> &Identifiers);
public:
----------------
Align `SmallVector` to `OpenMPDirectiveKind`.
================
Comment at: lib/Parse/ParseOpenMP.cpp:62
@@ +61,3 @@
+/// ParseOpenMPSimpleVarList
+/// (simple-variable-list);
+///
----------------
This comment is not helpful at all -- what's a `simple-variable-list`? I also don't see it in the OpenMP spec.
================
Comment at: lib/Sema/SemaOpenMP.cpp:9-11
@@ +8,5 @@
+//===----------------------------------------------------------------------===//
+//
+// This file implements semantic analysis for OpenMP directives and clauses
+//
+//===----------------------------------------------------------------------===//
----------------
Please use `\file` and three slashes.
================
Comment at: include/clang/Sema/Sema.h:6537-6539
@@ +6536,5 @@
+ /// \brief Called on well-formed '\#pragma omp threadprivate'.
+ DeclGroupPtrTy ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
+ Scope *CurScope,
+ const ArrayRef<DeclarationNameInfo> &Identifiers);
+
----------------
Don't leave `Loc` on the previous line. Also, drop `const` from `ArrayRef`, since all its methods are already `const`.
Same for the function definition.
================
Comment at: lib/Parse/ParseOpenMP.cpp:56
@@ +55,3 @@
+ SkipUntil(tok::semi);
+ break;;
+ }
----------------
Extra semicolon.
================
Comment at: lib/Parse/ParseOpenMP.cpp:33-34
@@ +32,4 @@
+ SmallVector<DeclarationNameInfo, 5> Identifiers;
+ switch(getOpenMPDirectiveKind(Tok)) {
+ case OMPD_threadprivate:
+ ConsumeToken();
----------------
LLVM style is not to indent `case` within a `switch`.
================
Comment at: lib/Parse/ParseOpenMP.cpp:66
@@ +65,3 @@
+ SmallVector<DeclarationNameInfo, 5> &Identifiers) {
+ // Parse '('
+ bool IsCorrect = true;
----------------
Please end all comments with a full stop.
================
Comment at: lib/Sema/SemaOpenMP.cpp:44
@@ +43,3 @@
+ SmallVector<VarDecl *, 5> Vars;
+ for (ArrayRef<DeclarationNameInfo>::const_iterator I = Identifiers.begin(),
+ E = Identifiers.end();
----------------
Use a simple `::iterator` -- they are the same for `ArrayRef`.
================
Comment at: lib/Sema/SemaOpenMP.cpp:63
@@ +62,3 @@
+ VarDecl *VD = Lookup.getAsSingle<VarDecl>();
+ NamedDecl *ND;
+ if (!VD) {
----------------
Move this declaration down to its initialization below.
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1630-1633
@@ +1629,6 @@
+ VisitDecl(D);
+ size_t numVars = D->varlist_size();
+ SmallVector<VarDecl *, 16> vars;
+ vars.reserve(numVars);
+ for (size_t i = 0; i != numVars; ++i) {
+ vars.push_back(ReadDeclAs<VarDecl>(Record, Idx));
----------------
`size_t` -> `unsigned`
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1630
@@ +1629,3 @@
+ VisitDecl(D);
+ size_t numVars = D->varlist_size();
+ SmallVector<VarDecl *, 16> vars;
----------------
Dmitri Gribenko wrote:
> `size_t` -> `unsigned`
`NumVars`
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1631
@@ +1630,3 @@
+ size_t numVars = D->varlist_size();
+ SmallVector<VarDecl *, 16> vars;
+ vars.reserve(numVars);
----------------
`Vars`
================
Comment at: lib/Serialization/ASTWriterDecl.cpp:125
@@ -124,2 +124,3 @@
void VisitObjCPropertyImplDecl(ObjCPropertyImplDecl *D);
+ void VisitOMPThreadPrivateDecl(OMPThreadPrivateDecl *LD);
};
----------------
`LD` -> `D`.
================
Comment at: lib/Serialization/ASTWriterDecl.cpp:1307-1308
@@ +1306,4 @@
+ VisitDecl(D);
+ for (OMPThreadPrivateDecl::varlist_iterator I = D->varlist_begin(),
+ E = D->varlist_end();
+ I != E; ++I)
----------------
Please align `E` to `I`.
================
Comment at: test/OpenMP/no_option.c:4
@@ +3,3 @@
+int a;
+#pragma omp threadprivate(a,b) // expected-warning {{unexpected '#pragma omp ...' in program}}
+#pragma omp parallel
----------------
I think the consensus was not to change the default behavior until our OpenMP implementation is somewhat useable.
================
Comment at: test/OpenMP/threadprivate_messages.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macos10.7.0 -verify -fopenmp -ferror-limit 100 -o - %s
+
----------------
Drop the `-o -`
http://llvm-reviews.chandlerc.com/D356
More information about the cfe-commits
mailing list