[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.
From:       Richard Smith <richard () metafoo ! co ! uk>
Date:       2015-07-31 21:07:26
Message-ID: 8c43fa54d4d20cab8a9bf5e43b073e25 () localhost ! localdomain
[Download RAW message or body]

rsmith added inline comments.

================
Comment at: lib/Parse/ParseOpenMP.cpp:171-177
@@ +170,9 @@
+
+    // Append the current token at the end of the new token stream so that it
+    // doesn't get lost.
+    CachedPragmas.push_back(Tok);
+    // Push back tokens for pragma.
+    PP.EnterTokenStream(CachedPragmas.data(), CachedPragmas.size(),
+                        /*DisableMacroExpansion=*/true,
+                        /*OwnsTokens=*/false);
+    // Parse pragma itself.
----------------
ABataev wrote:
> rsmith wrote:
> > Why are you doing this delayed parsing? You still seem to have no tests that \
> > require it.
> We need it for future parsing of clauses associated with the 'declare simd' \
> construct. Some of the clauses may have references to function arguments. That's \
> why I'm using delayed parsing: at first we need to parse a function along with \
> arguments and only then we'll parse all the pragmas along with their clauses and \
> references to args. Of course currently it is not used, because this patch does not \
> introduce parsing of clauses. It will be added later.
This code won't work for that; you've left the scope of the function parameters, so \
lookup for them will fail here. Generally-speaking, we don't like speculative / \
untested code to be committed, and would prefer to hold back on those changes until \
we can actually test them in some way.

Here's what I suggest: remove the delayed parsing code from here for this commit \
(parse the pragma first, then parse the nested declaration, then act on the result), \
and bring that code back once you introduce parsing for a clause that needs it, if \
indeed that's the right approach for those clauses. (As I mentioned before, if all \
they do is to provide a list of identifiers naming parameters, you don't need delay \
parsing and can instead store them as identifiers and look them up in Sema after the \
fact. On the other hand, if this pragma allows an arbitrary expression referencing a \
parameter to appear before the declaration of that parameter, then we'll need \
something like delayed parsing.)


http://reviews.llvm.org/D10599




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]