On Wed, Jan 30, 2013 at 5:16 PM, Alexey Bataev <a.bat...@hotmail.com> wrote:
> Hello all, Doug,
> Here is the second patch for OpenMP support in clang. It includes parsing
> and semantic analysis for #pragma omp threadprivate. It would be good if you
> take a look at it and send any comments.
Hello Alexey,
I am excited to see this patch!
This is not a full review. It might be easier to do reviews if you
uploaded the patch to phabricator. (But I don't know Doug's
preference about this.)
The list of warnings below should NEVER grow. It should gradually shrink to 0.
-CHECK: Warnings without flags (145):
+CHECK: Warnings without flags (146):
That comment says what it says. We should never add a warnings
without a flag, even temporarily.
\ No newline at end of file
Please add a newline.
Index: test/OpenMP/threadprivate_messages.cpp
===================================================================
--- test/OpenMP/threadprivate_messages.cpp (revision 0)
+++ test/OpenMP/threadprivate_messages.cpp (revision 0)
threadprivate_diagnostics might be better.
It is bikeshedding, it might be better to name all entities in the
test using some consistent scheme.
+int a; // expected-note {{forward declaration of 'a'}}
This is not correct. This is not a forward declaration, this is a
tentative definition in C, and just a plain definition in C++
+static __thread int thread_local; // expected-note {{forward
declaration of 'thread_local'}}
'thread_local' is a keyword in C++11. To future-proof this test,
choose a different name.
+#pragma omp threadprivate (staticVar) // expected-error {{undeclared
variable 'staticVar' used as an argument for '#pragma omp
threadprivate'}}
If it is not declared, how do we know it is a variable?
+#pragma omp threadprivate(argc,ll) // expected-error 2 {{arguments of
'#pragma omp threadprivate' must have global storage}}
There is no 'global' storage. 'static storage duration' is needed here.
+++ include/clang/Basic/OpenMPKinds.h (revision 0)
+class Token;
[...]
+OpenMPDirectiveKind getOpenMPDirectiveKind(Token Tok);
This header does not compile when it is the first one included in the TU.
+def warn_pragma_omp_ignored : Warning <
+ "unexpected '#pragma omp ...' in program; did you forget one of '-fopenmp' "
+ "or '-fno-openmp' flags?">;
I think the consensus was that until our implementation is somewhat
complete, we should not support or suggest -fopenmp in the driver.
+ /// ActOnOpenMPThreadprivateDirective - Called on well-formed
+ /// '\#pragma omp threadprivate'.
+ DeclGroupPtrTy ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
When adding new code, please don't duplicate function or class name
name in the comment. (There are multiple occurrences of this.)
+++ include/clang/AST/OpenMP.h (revision 0)
I think this should be DeclOpenMP, so it is named like DeclCXX,
DeclTemplate, etc.
+// This file defines the OpenMP directives.
"This file defines OpenMP AST nodes." And please also use three
slashes and "\file".
+ varlist_iterator varlist_end() {return Vars + NumVars;}
Spaces after "{" and before "}". (There are multiple occurrences of this.)
+ // Implement isa/cast/dyncast/etc.
You can safely drop this comment.
+ class OpenMPVarDeclBitfields {
+ friend class VarDecl;
+ friend class ASTDeclReader;
+
+ unsigned : 32;
+
+ /// \brief is the variable threadprivate
+ unsigned Threadprivate : 1;
+ };
Why did you leave a whole word of padding here?
+ for (OMPThreadPrivateDecl::varlist_iterator P = D->varlist_begin(),
+ E = D->varlist_end();
Coding style: iterator should be named I, and 'I' and 'E' are usually aligned.
+OpenMPDirectiveKind clang::getOpenMPDirectiveKind(Token Tok) {
+ return (llvm::StringSwitch<OpenMPDirectiveKind>(
+ Tok.getIdentifierInfo()->getName())
Why is this parenthesized?
+ case (OMPD_unknown):
Please don't parenthesize 'case' values.
+/// ActOnOpenMPThreadprivateDirective - Called on well-formed
+/// '\#pragma omp threadprivate'.
+Sema::DeclGroupPtrTy
Sema::ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
Don't duplicate the comment in .cpp file.
+Sema::DeclGroupPtrTy
Sema::ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
+ Scope *CurScope,
+ const SmallVector<Token, 5> &Identifiers) {
This should be SmallVectorImpl, or even better, an ArrayRef.
--- lib/AST/OpenMP.cpp (revision 0)
+++ lib/AST/OpenMP.cpp (revision 0)
@@ -0,0 +1,53 @@
+//===--- Decl.cpp - Declaration AST Node Implementation
-------------------===//
This comment is not correct.
+// This file implements the Decl subclasses.
This one too.
+void OMPThreadPrivateDecl::setVars(ASTContext &Context, VarDecl **B,
+ const size_t S) {
(1) Align "ASTContext" and "const".
(2) Don't constify by-value parameters.
+ size_t allocationSize = S * sizeof(VarDecl *);
+ void *buffer = Context.Allocate(allocationSize, sizeof(void *));
(1) Use llvm::alignOf<>.
(2) Tail-allocate instead of doing an additional allocation. See
CXXTryStmt::Create as an example.
+ else if (isa<FunctionDecl>(*D) &&
cast<FunctionDecl>(*D)->isThisDeclarationADefinition())
Align 'isa' and 'cast'.
+//----------------------------------------------------------------------------
+// OpenMP #pragma omp threadprivate
+//----------------------------------------------------------------------------
This kind of ASCII art is discouraged, especially when used to mark a
single function.
+ Token *Toks = new Token[Pragma.size()];
That's a memleak.
+ void SetWarned() {Warned = true;}
Function names should start with a lowercase letter. Also missing
spaces around "{}".
+ SourceLocation Loc = ConsumeToken();
+ switch(getOpenMPDirectiveKind(Tok)) {
+ case (OMPD_threadprivate): {
+ SmallVector<Token, 5> Identifiers;
Weird indentation on "SmallVector" and "case".
+ if(ParseOpenMPSimpleVarList(OMPD_threadprivate, Identifiers)) {
Space after 'if'.
+ return (Actions.ActOnOpenMPThreadprivateDirective(Loc,
+ getCurScope(),
+ Identifiers));
Weird indentation and parentheses.
+ // Read tokens while ')' or ';' is not found
Why are you doing special processing for a semicolon?
+ }
+ else {
Should be "} else {".
+ bool isCorrect = true;
Variable names should start with an uppercase letter.
+ // A threadprivate directive for static class member variables must appear
+ // in the class definition, in the same scope in which the member
+ // variables are declared
A reference would not hurt here. Also, full stop at the end, and
empty lines to separate paragraphs.
Also, I don't see where is this rule checked. If it is, then could
you add a test:
struct A {
static int a;
};
#pragma omp threadprivate (A::a)
+ // A threadprivate directive for namespace-scope variables must appear
+ // outside any definition or declaration other than the namespace
+ // definition itself
I don't see where this is checked, too. And tests are apparently missing.
Dmitri
--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <griboz...@gmail.com>*/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
