[PATCH] OpenMP threadprivate directive
Alexey Bataev
a.bataev at hotmail.com
Mon Feb 25 20:22:57 PST 2013
================
Comment at: include/clang/AST/Decl.h:773
@@ -772,1 +772,3 @@
+ class OpenMPVarDeclBitfields {
+ friend class VarDecl;
----------------
Doug Gregor wrote:
> Why is this a separate set of bitfields? What indicates when we should be using OpenMPVarDeclBitfields rather than VarDeclBitfields?
>
> If this is to be a bit, it should be OpenMPThreadPrivate bit on VarDeclBitfields.
After some thoughts, I've decided to remove this bit from declarations for now. Most probably I can elaborate threadprivate variables without this bit or I'll add later, when it will be necessary.
================
Comment at: include/clang/AST/DeclOpenMP.h:24
@@ +23,3 @@
+/// OMPThreadPrivateDecl - This represents #pragma omp threadprivate ...
+/// directive
+class OMPThreadPrivateDecl : public Decl {
----------------
Doug Gregor wrote:
> The comment should show how the pragma is used with an actual code snippet. Mentioning that the actual threadprivate-ness of the referenced variables is also recorded on the VarDecl would be nice, too.
Ok, I'll add the comment
================
Comment at: include/clang/AST/DeclOpenMP.h:25
@@ +24,3 @@
+/// directive
+class OMPThreadPrivateDecl : public Decl {
+ size_t NumVars;
----------------
Doug Gregor wrote:
> This class is keeping track of the set of VarDecls being made thread-private, but it loses all source-location information about the actual in-source text used to refer to those variables. We'd like to keep that information (e.g., as DeclarationNameInfos), both because it's good for other tools (e.g., finding all references to a variable) and because you're going to need those locations for diagnostics when you implement template instantiation for this decl.
Agree, I will use DeclRefExprs instead of VarDecls
================
Comment at: include/clang/AST/DeclOpenMP.h:33
@@ +32,3 @@
+
+ VarDecl const * const *getVars() const {
+ return reinterpret_cast<VarDecl const * const *>(this + 1);
----------------
Doug Gregor wrote:
> Dmitri Gribenko wrote:
> > `const VarDecl` is more in line with LLVM style.
> An ArrayRef would be a much nicer return value here.
Ok
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:773
@@ +772,3 @@
+def warn_pragma_omp_ignored : Warning <
+ "unexpected '#pragma omp ...' in program">, InGroup<SourceUsesOpenMP>;
+def err_omp_unknown_directive : Error <
----------------
Doug Gregor wrote:
> This warning (and all warnings in -Wsource-uses-openmp) should be DefaultIgnore. When OpenMP is fully-supported, we can consider turning these warnings on by default and suggesting -fopenmp.
Agree, fixed.
================
Comment at: lib/Parse/ParseOpenMP.cpp:42
@@ +41,3 @@
+ SkipUntil(tok::semi);
+ break;
+ }
----------------
Doug Gregor wrote:
> Why should having extra tokens cause us to drop the threadprivate directive entirely? It would be better recovery to just continue on.
Ok, changed to generate warning
================
Comment at: lib/Parse/ParseOpenMP.cpp:86
@@ +85,3 @@
+ }
+ if (ParseUnqualifiedId(SS, false, false, false, ParsedType(),
+ TemplateKWLoc, Name)) {
----------------
Doug Gregor wrote:
> Is a simple-variable-list supposed to allow qualified names?
According to OpenMP 4.0 RC 1 (2.11.2, Restrictions):
A threadprivate directive for namespace-scope variables must appear outside any definition or declaration other than the namespace definition itself, and must lexically precede all references to any of the variables in its list.
So qualified names are not allowed, because directive should appear only in the context of namespace.
================
Comment at: lib/AST/DeclOpenMP.cpp:50
@@ +49,3 @@
+ SourceLocation L, VarDecl **VL,
+ size_t N)
+ : Decl(DK, DC, L), NumVars(N) {
----------------
Doug Gregor wrote:
> Please use ArrayRef<VarDecl*> rather than VarDecl**/size_t. It's not everywhere in the Clang code base, but it's the preferred style now.
Ok
================
Comment at: lib/AST/DeclOpenMP.cpp:58
@@ +57,3 @@
+
+void OMPThreadPrivateDecl::setVars(VarDecl **VL) {
+ if (VL) {
----------------
Doug Gregor wrote:
> ArrayRef here, please.
Ok
================
Comment at: lib/Parse/ParseOpenMP.cpp:76
@@ +75,3 @@
+ UnqualifiedId Name;
+
+ // Read tokens while ')' or ';' is not found
----------------
Doug Gregor wrote:
> I'd feel more comfortable moving these into the loop, right before the ParseUnqualifiedId, so it's clear that they are meant to be fresh each time we look for a new identifier.
Ok
================
Comment at: lib/Parse/ParseOpenMP.cpp:95
@@ +94,3 @@
+ SkipUntil(tok::comma, tok::r_paren, true, true);
+ Diag(PrevTok.getLocation(), diag::err_expected_unqualified_id) <<
+ getLangOpts().CPlusPlus <<
----------------
Doug Gregor wrote:
> LLVM style prefers that the <<'s go at the beginning of lines, not at the end. This occurs a number of times throughout the patch.
Ok, fixed
================
Comment at: lib/Parse/ParseOpenMP.cpp:107
@@ +106,3 @@
+ T.consumeClose();
+ return IsCorrect;
+}
----------------
Doug Gregor wrote:
> LLVM convention is to return false for success, true for failure.
>
> Also, why not return success so long as we've managed to parse any names? Then we'll get more semantic analysis done.
Ok
================
Comment at: lib/Parse/ParsePragma.h:102
@@ -101,1 +101,3 @@
+class PragmaOpenMPHandler : public PragmaHandler {
+ bool Warned;
----------------
Doug Gregor wrote:
> This is really two handlers masquerading as one: the real OpenMP handler and the "complain about anything that looks like OpenMP" handler. They should be separate classes.
Agree, I'll add the new one
================
Comment at: lib/Parse/ParsePragma.cpp:733
@@ +732,3 @@
+ PP.Diag(FirstTok, diag::warn_pragma_omp_ignored);
+ }
+ PP.DiscardUntilEndOfDirective();
----------------
Doug Gregor wrote:
> We usually try to avoid such "already warned" bits in Clang, because we don't know (without checking explicitly) whether a call to Diag() will actually emit the warning. For example, if there were a #pragma omp threadprivate in a system header, that warning would be suppressed and the user would never see a warning.
>
> I suggest removing the iswarned/setwarned stuff. There's nothing like a flood of "I'm ignoring this" warnings to get a user's attention.
Doug, there was a long discussion about warnings. And the final conclusion was that the warning for OpenMP directives should be generated only once when there is no -fopenmp option.
================
Comment at: lib/Parse/ParsePragma.cpp:751
@@ +750,3 @@
+ Tok.startToken();
+ Tok.setKind(tok::semi);
+ Tok.setLocation(EodLoc);
----------------
Doug Gregor wrote:
> tok::semi is a poor choice of delimiter here, because it could show up in an (ill-formed) pragma. Why not use tok::eod, which is reserved for this purpose?
There are some problems with tok::eod. When I try to enter tokens with tok::eod at the end Preprocessor::HandlePragmaDirective method throws them away because of the following code
// If the pragma handler didn't read the rest of the line, consume it now.
if ((CurTokenLexer && CurTokenLexer->isParsingPreprocessorDirective())
|| (CurPPLexer && CurPPLexer->ParsingPreprocessorDirective))
DiscardUntilEndOfDirective();
It sees, that there is tok::eod at the end of the CurTokenLexer and CurTokenLexer->isParsingPreprocessorDirective() returns true. All tokens then are discarded by DiscardUntilEndOfDirective(). Besides, some of the language construct require ';' at the end. For example, #pragma omp threadprivate (if parsed as a special kind statement) in the context of class declaration should be terminated by the ';'
Possible solution is to use as a terminator some annotation symbol like annot_pragma_openmp_end and continue parsing until this annotation is found.
================
Comment at: lib/Parse/ParseStmt.cpp:292
@@ +291,3 @@
+
+ case tok::annot_pragma_openmp:
+ SourceLocation DeclStart = Tok.getLocation();
----------------
Doug Gregor wrote:
> Why isn't this handled in the default case, if the pragma will always end up being a declaration? Or do you expect this to get more interesting in the future?
Yes, there will be parsing of executable OpenMP directives too, which are not a declaration, but a kind of statement.
================
Comment at: lib/Sema/SemaOpenMP.cpp:31
@@ +30,3 @@
+ if (NamedDecl *ND = Candidate.getCorrectionDecl()) {
+ return isa<VarDecl>(ND) &&
+ Actions.isDeclInScope(ND, Actions.getCurLexicalContext(),
----------------
Doug Gregor wrote:
> Do you also want to check for static storage duration, or do you want to allow corrections to variables with automatic storage (and, therefore, emit an error on those corrections)? I suggest the former.
Agree, also checking for global storage.
================
Comment at: lib/Sema/SemaOpenMP.cpp:49
@@ +48,3 @@
+ FilterLookupForScope(Lookup, getCurLexicalContext(), CurScope,
+ false, false);
+ LookupParsedName(Lookup, CurScope, NULL, true);
----------------
Doug Gregor wrote:
> It doesn't make sense to filter the lookup results before you've done the actual lookup. Besides, you don't want to filter things that are out-of-scope, you want to detect them later on and complain about that mistake directly.
Agree, removed
================
Comment at: lib/Sema/SemaOpenMP.cpp:58
@@ +57,3 @@
+ VarDeclFilterCCC Validator(*this);
+ DiagnoseEmptyLookup(CurScope, ScopeSpec, Lookup, Validator);
+ continue;
----------------
Doug Gregor wrote:
> DiagnoseEmptyLookup recovers using typo correction when possible. You need to detect that case and act as if the user had written the right thing, not "continue" as you would if no correction occurred.
Fixed
================
Comment at: lib/Sema/SemaOpenMP.cpp:63
@@ +62,3 @@
+ VarDecl *VD = Lookup.getAsSingle<VarDecl>();
+ NamedDecl *ND;
+ if (!VD) {
----------------
Dmitri Gribenko wrote:
> Move this declaration down to its initialization below.
Done
================
Comment at: lib/Sema/SemaOpenMP.cpp:73
@@ +72,3 @@
+ FixItHint::CreateReplacement(I->getLoc(), CorrectedStr);
+ continue;
+ }
----------------
Doug Gregor wrote:
> This is an odd bit of recovery. If name lookup finds something that isn't a VarDecl, it's attempting to typo correct? Shouldn't it simply complain if the thing found (if there is one) is not a VarDecl, and point at what it did find?
>
> DiagnoseEmptyLookup should handle the "nothing found" case.
Doug, this code tries to produce message when variable lookup results in an ambiguity.
================
Comment at: lib/Sema/SemaOpenMP.cpp:104
@@ +103,3 @@
+ // A threadprivate directive for static block-scope variables must appear
+ // in the scope of the variable and not in a nested scope.
+ if (!isDeclInScope(ND, getCurLexicalContext(), CurScope)) {
----------------
Doug Gregor wrote:
> So all of these paragraphs are essentially one check. That's handy, but this formatting makes it read more like we're missing checks. How about eliminating the whitespace between comments and indenting the actual parts that are excerpted (which better matches what we do everywhere else in Clang anyway). For example:
>
> // OpenMP [2.9.2, Restrictions, C/C++, p.2]
> // A threadprivate directive for file-scope variables must appear outside
> // any definition or declaration.
> // OpenMP [2.9.2, Restrictions, C/C++, p.3]
> // 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.
Fixed
================
Comment at: lib/Sema/SemaOpenMP.cpp:124
@@ +123,3 @@
+ if (RequireCompleteType(I->getLoc(), VD->getType(),
+ diag::err_incomplete_type)) {
+ continue;
----------------
Doug Gregor wrote:
> I suggest customizing the error diagnostic for the incomplete-type case.
Ok
================
Comment at: lib/Sema/SemaOpenMP.cpp:142
@@ +141,3 @@
+ getOpenMPDirectiveName(OMPD_threadprivate) <<
+ VD->getType().getUnqualifiedType();
+ continue;
----------------
Doug Gregor wrote:
> What is this trying to check? The restriction here is saying that the address of a thread-private variable is not address-constant. That affects expressions using the variable, not the variable itself. See, e.g., VarDecl::isUsableInConstantExpressions.
Oh, good catch. I incorrectly interpreted the sentence. So, it seems that we don't need additional checking for this because it is covered by reference type checking.
================
Comment at: include/clang/AST/Decl.h:780
@@ +779,3 @@
+ /// \brief is the variable threadprivate
+ unsigned Threadprivate : 1;
+ };
----------------
Doug Gregor wrote:
> This is bloating VarDecls by an extra 4 bytes for a single bit of storage. That doesn't make sense; see above.
Removed
================
Comment at: include/clang/AST/Decl.h:883
@@ -867,1 +882,3 @@
+ }
+
/// hasLocalStorage - Returns true if a variable with function scope
----------------
Doug Gregor wrote:
> Why is this a bit, rather than a storage class? If it's a storage class, then we're forced to think about and make the right decision whether something is querying the storage class. If it's a bit, it's easily forgotten, e.g., the missing address-constant logic.
Removed
================
Comment at: include/clang/AST/DeclOpenMP.h:26
@@ +25,3 @@
+class OMPThreadPrivateDecl : public Decl {
+ size_t NumVars;
+
----------------
Dmitri Gribenko wrote:
> 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.)
>
Fixed
================
Comment at: include/clang/AST/DeclOpenMP.h:33-34
@@ +32,4 @@
+
+ VarDecl const * const *getVars() const {
+ return reinterpret_cast<VarDecl const * const *>(this + 1);
+ }
----------------
Alexey Bataev wrote:
> Doug Gregor wrote:
> > Dmitri Gribenko wrote:
> > > `const VarDecl` is more in line with LLVM style.
> > An ArrayRef would be a much nicer return value here.
> Ok
Fixed
================
Comment at: include/clang/AST/DeclOpenMP.h:49
@@ +48,3 @@
+ typedef VarDecl **varlist_iterator;
+ typedef VarDecl const * const *varlist_const_iterator;
+
----------------
Dmitri Gribenko wrote:
> `const VarDecl`
Fixed
================
Comment at: include/clang/AST/DeclOpenMP.h:58
@@ +57,3 @@
+
+ void setVars(VarDecl **B);
+
----------------
Dmitri Gribenko wrote:
> Please make this private and add `friend class ASTDeclReader`.
>
> Also, what does parameter name `B` stand for? The definition of this function uses `VL`.
>
Fixed
================
Comment at: include/clang/AST/DeclVisitor.h:21
@@ -20,2 +20,3 @@
#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclOpenMP.h"
----------------
Dmitri Gribenko wrote:
> Please sort includes alphabetically.
>
Fixed
================
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"
----------------
Dmitri Gribenko wrote:
> Please sort includes alphabetically.
Fixed
================
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) {
----------------
Dmitri Gribenko wrote:
> Align `P` and `E`. Also, `I` is more in line with LLVM style.
>
> Same for `tools/libclang/RecursiveASTVisitor.h`.
Aligned and renamed P to I
================
Comment at: include/clang/Basic/LangOptions.h:67
@@ -66,2 +66,3 @@
};
+ enum OpenMPMode { OpenMPModeNotSet, OpenMPModeOff, OpenMPModeOn };
----------------
Doug Gregor wrote:
> I don't think this should be a three-state mode. The three states you want seem to be:
>
> - Default: Complain if we see OpenMP (presumably because the user forgot to pass -fopenmp)
> - On: we have OpenMP support
> - Off: we don't have OpenMP support and don't complain
>
> However, the difference between Default and Off is better handled in the diagnostics system. -fopenmp turns on OpenMP. -Wno-source-uses-openmp will turn off the diagnostics if one wants to compile without OpenMP and not see complains about it. See additional comment about this on the diagnostics bits.
Fixed, returned to two-state option
================
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"
----------------
Dmitri Gribenko wrote:
> Please sort includes alphabetically.
Fixed
================
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:
----------------
Dmitri Gribenko wrote:
> Align `SmallVector` to `OpenMPDirectiveKind`.
Fixed, changed type to SmallVectorImpl
================
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);
+
----------------
Doug Gregor wrote:
> Dmitri Gribenko wrote:
> > 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.
> >
> ArrayRefs should be passed by value, not be reference.
Fixed, passed ArrayRef by value and aligned properly.
================
Comment at: lib/Frontend/InitPreprocessor.cpp:645
@@ -644,3 +644,3 @@
// OpenMP definition
- if (LangOpts.OpenMP) {
+ if (LangOpts.getOpenMP()) {
// OpenMP 2.2:
----------------
Doug Gregor wrote:
> This still triggers with -fno-openmp, doesn't it? Eliminating the three-state OpenMP mode would solve the problem.
Changed to two-state
================
Comment at: lib/Parse/ParseOpenMP.cpp:33-34
@@ +32,4 @@
+ SmallVector<DeclarationNameInfo, 5> Identifiers;
+ switch(getOpenMPDirectiveKind(Tok)) {
+ case OMPD_threadprivate:
+ ConsumeToken();
----------------
Dmitri Gribenko wrote:
> LLVM style is not to indent `case` within a `switch`.
Fixed
================
Comment at: lib/Parse/ParseOpenMP.cpp:56
@@ +55,3 @@
+ SkipUntil(tok::semi);
+ break;;
+ }
----------------
Dmitri Gribenko wrote:
> Extra semicolon.
>
Removed
================
Comment at: lib/Parse/ParseOpenMP.cpp:66
@@ +65,3 @@
+ SmallVector<DeclarationNameInfo, 5> &Identifiers) {
+ // Parse '('
+ bool IsCorrect = true;
----------------
Dmitri Gribenko wrote:
> Please end all comments with a full stop.
Fixed
================
Comment at: lib/Sema/SemaOpenMP.cpp:44
@@ +43,3 @@
+ SmallVector<VarDecl *, 5> Vars;
+ for (ArrayRef<DeclarationNameInfo>::const_iterator I = Identifiers.begin(),
+ E = Identifiers.end();
----------------
Dmitri Gribenko wrote:
> Use a simple `::iterator` -- they are the same for `ArrayRef`.
Fixed
================
Comment at: lib/Sema/SemaOpenMP.cpp:146
@@ +145,3 @@
+
+ // Check if threadspecified is set
+ if (VD->isThreadSpecified()) {
----------------
Doug Gregor wrote:
> There's a reference for this, is there not?
Noop, there is not. Actually, most of implementations just marks threadprivate variables as thread-local. But TLS is not supported on some platforms. I want to try to elaborate threadprivate variables without marking them as thread-local. But thread-local variables should not be marked as threadprivate, because they already must be replicated between different threads.
================
Comment at: lib/Sema/SemaOpenMP.cpp:156
@@ +155,3 @@
+ }
+ if (!Vars.empty()) {
+ OMPThreadPrivateDecl *OTD = OMPThreadPrivateDecl::Create(Context,
----------------
Doug Gregor wrote:
> I suggest flipping this conditional around, to de-nest more code:
>
> if (Vars.empty())
> return DeclGroupPtrTy();
>
> // create the thread-private decl.
Fixed
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:309
@@ -308,2 +308,3 @@
Var->setConstexpr(D->isConstexpr());
+ Var->setOMPThreadprivate(D->isOMPThreadprivate());
----------------
Doug Gregor wrote:
> I don't think instantiation should propagate the threadprivate bit directly. Rather, when one instantiates the OpenMPThreadPrivateDecl, it will perform the semantic analysis again (since we'll have concrete types) and set the threadprivate bit. Obviously, Sema::ActOnOpenMPThreadPrivate will need to be split into n ActOn and a Check method, as we do for most other semantic analysis.
Removed
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1630
@@ +1629,3 @@
+ VisitDecl(D);
+ size_t numVars = D->varlist_size();
+ SmallVector<VarDecl *, 16> vars;
----------------
Dmitri Gribenko wrote:
> Dmitri Gribenko wrote:
> > `size_t` -> `unsigned`
> `NumVars`
Fixed
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1631
@@ +1630,3 @@
+ size_t numVars = D->varlist_size();
+ SmallVector<VarDecl *, 16> vars;
+ vars.reserve(numVars);
----------------
Dmitri Gribenko wrote:
> `Vars`
Fixed
================
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));
----------------
Alexey Bataev wrote:
> Dmitri Gribenko wrote:
> > Dmitri Gribenko wrote:
> > > `size_t` -> `unsigned`
> > `NumVars`
> Fixed
Fixed
================
Comment at: lib/Serialization/ASTWriterDecl.cpp:125
@@ -124,2 +124,3 @@
void VisitObjCPropertyImplDecl(ObjCPropertyImplDecl *D);
+ void VisitOMPThreadPrivateDecl(OMPThreadPrivateDecl *LD);
};
----------------
Dmitri Gribenko wrote:
> `LD` -> `D`.
Fixed
================
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)
----------------
Dmitri Gribenko wrote:
> Please align `E` to `I`.
Aligned
================
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
----------------
Dmitri Gribenko wrote:
> I think the consensus was not to change the default behavior until our OpenMP implementation is somewhat useable.
Fixed
================
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
+
----------------
Dmitri Gribenko wrote:
> Drop the `-o -`
Fixed
================
Comment at: include/clang/AST/DeclOpenMP.h:28
@@ +27,3 @@
+///
+/// @code
+/// int a;
----------------
Dmitri Gribenko wrote:
> Please use backslashes for all doxygen commands (for consistency).
Ok
================
Comment at: include/clang/AST/DeclOpenMP.h:52
@@ +51,3 @@
+
+ llvm::MutableArrayRef<DeclRefExpr *> getVars() {
+ return llvm::MutableArrayRef<DeclRefExpr *>(
----------------
Dmitri Gribenko wrote:
> The difference between MutableArrayRef and ArrayRef here is that MutableArrayRef would allow users to modify the *pointers* themselves (and I don't think this is a good idea). ArrayRef should probably be used here.
The problem is that ArrayRef has only const iterators, while sometimes it is required to have non-const iterators. That's why I had to add getVars() method which returns MutableArrayRef.
================
Comment at: lib/Parse/ParseOpenMP.cpp:68
@@ +67,3 @@
+/// directive
+/// ( unqualified-id {, unqualified-id} ) annot_pragma_openmp_end
+///
----------------
Dmitri Gribenko wrote:
> Please add production name here: `simple-variable-list`.
Ok
================
Comment at: lib/Parse/ParsePragma.cpp:727-729
@@ +726,5 @@
+ Token &FirstTok) {
+ switch (PP.getDiagnostics().getDiagnosticLevel(diag::warn_pragma_omp_ignored,
+ FirstTok.getLocation())) {
+ case DiagnosticsEngine::Ignored:
+ break;
----------------
Dmitri Gribenko wrote:
> You cloud replace the switch with a `!=` comparison with `Ignored`?
Yes, of course
================
Comment at: lib/Parse/Parser.cpp:101-102
@@ +100,4 @@
+ OpenMPHandler.reset(new PragmaOpenMPHandler());
+ }
+ else {
+ OpenMPHandler.reset(new PragmaNoOpenMPHandler());
----------------
Dmitri Gribenko wrote:
> These braces should be on the same line. Or better, just drop them.
>
Ok
================
Comment at: lib/Sema/SemaOpenMP.cpp:23
@@ +22,3 @@
+
+
+namespace {
----------------
Dmitri Gribenko wrote:
> Extra empty line?
Removed
================
Comment at: lib/Sema/SemaOpenMP.cpp:33-34
@@ +32,4 @@
+ if (NamedDecl *ND = Candidate.getCorrectionDecl()) {
+ return isa<VarDecl>(ND) &&
+ cast<VarDecl>(ND)->hasGlobalStorage() &&
+ Actions.isDeclInScope(ND, Actions.getCurLexicalContext(),
----------------
Dmitri Gribenko wrote:
> You can fuse this isa/cast pair to the previous line using `dyn_cast_or_null`.
>
Ok
================
Comment at: lib/Sema/SemaOpenMP.cpp:100
@@ +99,3 @@
+ // in the scope of the variable and not in a nested scope.
+ NamedDecl *ND = cast<NamedDecl>(VD);
+ if (!isDeclInScope(ND, getCurLexicalContext(), CurScope)) {
----------------
Dmitri Gribenko wrote:
> VarDecl is derived from ValueDecl, which is derived from NamedDecl, there is no need for this variable.
>
No, this one required for isDeclInScope() method, because it accepts NamedDecl *& as the first parameter. Neither clang, nor gcc is able to compile isDeclInScope(VD, ...).
================
Comment at: lib/Sema/SemaOpenMP.cpp:124-125
@@ +123,4 @@
+ }
+ if (OMPThreadPrivateDecl *D = CheckOMPThreadPrivateDecl(Loc,
+ Vars)) {
+ CurContext->addDecl(D);
----------------
Dmitri Gribenko wrote:
> Weird line break.
Fixed
================
Comment at: lib/Parse/ParsePragma.cpp:721
@@ -720,1 +720,3 @@
+/// \brief Handle '#pragma omp ...' when no '-fopenmp' is provided.
+///
----------------
Dmitri Gribenko wrote:
> It might be better to explain this in terms of LangOpt flags, or just with a general "OpenMP is enabled".
Fixed
http://llvm-reviews.chandlerc.com/D356
More information about the cfe-commits
mailing list