Looks like a good start; see comments for a number of minor things and a few
more interesting design questions (esp. about the thread-private bit).
================
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 <
----------------
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.
================
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);
+
----------------
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.
================
Comment at: lib/AST/DeclOpenMP.cpp:50
@@ +49,3 @@
+ SourceLocation L, VarDecl **VL,
+ size_t N)
+ : Decl(DK, DC, L), NumVars(N) {
----------------
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.
================
Comment at: lib/AST/DeclOpenMP.cpp:58
@@ +57,3 @@
+
+void OMPThreadPrivateDecl::setVars(VarDecl **VL) {
+ if (VL) {
----------------
ArrayRef here, please.
================
Comment at: lib/Frontend/InitPreprocessor.cpp:645
@@ -644,3 +644,3 @@
// OpenMP definition
- if (LangOpts.OpenMP) {
+ if (LangOpts.getOpenMP()) {
// OpenMP 2.2:
----------------
This still triggers with -fno-openmp, doesn't it? Eliminating the three-state
OpenMP mode would solve the problem.
================
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 <<
----------------
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.
================
Comment at: lib/Parse/ParsePragma.cpp:751
@@ +750,3 @@
+ Tok.startToken();
+ Tok.setKind(tok::semi);
+ Tok.setLocation(EodLoc);
----------------
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?
================
Comment at: lib/Parse/ParseStmt.cpp:292
@@ +291,3 @@
+
+ case tok::annot_pragma_openmp:
+ SourceLocation DeclStart = Tok.getLocation();
----------------
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?
================
Comment at: lib/Sema/SemaOpenMP.cpp:49
@@ +48,3 @@
+ FilterLookupForScope(Lookup, getCurLexicalContext(), CurScope,
+ false, false);
+ LookupParsedName(Lookup, CurScope, NULL, true);
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:58
@@ +57,3 @@
+ VarDeclFilterCCC Validator(*this);
+ DiagnoseEmptyLookup(CurScope, ScopeSpec, Lookup, Validator);
+ continue;
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:73
@@ +72,3 @@
+ FixItHint::CreateReplacement(I->getLoc(), CorrectedStr);
+ continue;
+ }
----------------
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.
================
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)) {
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:124
@@ +123,3 @@
+ if (RequireCompleteType(I->getLoc(), VD->getType(),
+ diag::err_incomplete_type)) {
+ continue;
----------------
I suggest customizing the error diagnostic for the incomplete-type case.
================
Comment at: lib/Sema/SemaOpenMP.cpp:142
@@ +141,3 @@
+ getOpenMPDirectiveName(OMPD_threadprivate) <<
+ VD->getType().getUnqualifiedType();
+ continue;
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:146
@@ +145,3 @@
+
+ // Check if threadspecified is set
+ if (VD->isThreadSpecified()) {
----------------
There's a reference for this, is there not?
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:309
@@ -308,2 +308,3 @@
Var->setConstexpr(D->isConstexpr());
+ Var->setOMPThreadprivate(D->isOMPThreadprivate());
----------------
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.
================
Comment at: include/clang/AST/Decl.h:773
@@ -772,1 +772,3 @@
+ class OpenMPVarDeclBitfields {
+ friend class VarDecl;
----------------
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.
================
Comment at: include/clang/AST/Decl.h:780
@@ +779,3 @@
+ /// \brief is the variable threadprivate
+ unsigned Threadprivate : 1;
+ };
----------------
This is bloating VarDecls by an extra 4 bytes for a single bit of storage. That
doesn't make sense; see above.
================
Comment at: include/clang/AST/Decl.h:883
@@ -867,1 +882,3 @@
+ }
+
/// hasLocalStorage - Returns true if a variable with function scope
----------------
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.
================
Comment at: include/clang/AST/DeclOpenMP.h:33
@@ +32,3 @@
+
+ VarDecl const * const *getVars() const {
+ return reinterpret_cast<VarDecl const * const *>(this + 1);
----------------
Dmitri Gribenko wrote:
> `const VarDecl` is more in line with LLVM style.
An ArrayRef would be a much nicer return value here.
================
Comment at: include/clang/AST/DeclOpenMP.h:24
@@ +23,3 @@
+/// OMPThreadPrivateDecl - This represents #pragma omp threadprivate ...
+/// directive
+class OMPThreadPrivateDecl : public Decl {
----------------
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.
================
Comment at: include/clang/AST/DeclOpenMP.h:25
@@ +24,3 @@
+/// directive
+class OMPThreadPrivateDecl : public Decl {
+ size_t NumVars;
----------------
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.
================
Comment at: include/clang/Basic/LangOptions.h:67
@@ -66,2 +66,3 @@
};
+ enum OpenMPMode { OpenMPModeNotSet, OpenMPModeOff, OpenMPModeOn };
----------------
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.
================
Comment at: lib/Parse/ParseOpenMP.cpp:42
@@ +41,3 @@
+ SkipUntil(tok::semi);
+ break;
+ }
----------------
Why should having extra tokens cause us to drop the threadprivate directive
entirely? It would be better recovery to just continue on.
================
Comment at: lib/Parse/ParseOpenMP.cpp:76
@@ +75,3 @@
+ UnqualifiedId Name;
+
+ // Read tokens while ')' or ';' is not found
----------------
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.
================
Comment at: lib/Parse/ParseOpenMP.cpp:86
@@ +85,3 @@
+ }
+ if (ParseUnqualifiedId(SS, false, false, false, ParsedType(),
+ TemplateKWLoc, Name)) {
----------------
Is a simple-variable-list supposed to allow qualified names?
================
Comment at: lib/Parse/ParseOpenMP.cpp:107
@@ +106,3 @@
+ T.consumeClose();
+ return IsCorrect;
+}
----------------
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.
================
Comment at: lib/Parse/ParsePragma.cpp:733
@@ +732,3 @@
+ PP.Diag(FirstTok, diag::warn_pragma_omp_ignored);
+ }
+ PP.DiscardUntilEndOfDirective();
----------------
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.
================
Comment at: lib/Parse/ParsePragma.h:102
@@ -101,1 +101,3 @@
+class PragmaOpenMPHandler : public PragmaHandler {
+ bool Warned;
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:31
@@ +30,3 @@
+ if (NamedDecl *ND = Candidate.getCorrectionDecl()) {
+ return isa<VarDecl>(ND) &&
+ Actions.isDeclInScope(ND, Actions.getCurLexicalContext(),
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:156
@@ +155,3 @@
+ }
+ if (!Vars.empty()) {
+ OMPThreadPrivateDecl *OTD = OMPThreadPrivateDecl::Create(Context,
----------------
I suggest flipping this conditional around, to de-nest more code:
if (Vars.empty())
return DeclGroupPtrTy();
// create the thread-private decl.
http://llvm-reviews.chandlerc.com/D356
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
