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