ABataev added inline comments.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1569-1593
+ int idx = -1;
+ if (Tok.isAnyIdentifier()) {
+ II = Tok.getIdentifierInfo();
+ llvm::StringSwitch<int> SS(II->getName());
+ for (const AssumptionClauseMappingInfo &ACMI : AssumptionClauseMappings)
{
+ if (ACMI.StartsWith)
+ SS.StartsWith(ACMI.Identifier, ++idx);
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > `Idx`
> > > > Better to outline it into function/lambda
> > > Which part? Assuming you mean the 9 line conditional above, I'm not
> > > convinced. Even if I do outline it, 4 lines will have to stay here and I
> > > don't see any reuse.
> > The part that searches over the table.
> > The part that searches over the table.
>
> I don't follow. Where is a search? The part that builds a string switch? line
> 1569 - 1579?
Yes.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3218
+ DeclContext *DC = DeclContexts.pop_back_val();
+ for (auto *SubDC : DC->decls()) {
+ if (auto *CTD = dyn_cast<ClassTemplateDecl>(SubDC)) {
----------------
I would also add a check that the decl is valid here before checking it and
applying the attributes.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3225
+ }
+ if (isa<CXXRecordDecl>(SubDC) || isa<NamespaceDecl>(SubDC)) {
+ DeclContexts.push_back(cast<DeclContext>(SubDC));
----------------
```
if (auto *SubCtx = dyn_cast<DeclContext>(SubDC)) {
DeclContexts.push_back(cast<DeclContext>(SubCtx));
continue;
}
```
?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3210
+
+ SmallVector<DeclContext *, 8> DeclContexts;
+ DeclContexts.push_back(CurContext);
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > What are you trying to do here? Scan all the decls in the current
> > > > context and mark all the functions with the assumption attributes?
> > > Pretty much, yeah. Find all function declarations and add the attribute.
> > > I'll add a comment.
> > Does it mean that for this code
> > ```
> > void foo();
> > #pragma omp assume ...
> > void bar();
> > #pragma omp end assume
> > ```
> > `foo()` must have the same assumptions like `bar()`? It looks to me,
> > currently we'll end up with the same attributes for both functions. But I'm
> > not sure that this is correct to mark the function outside of the `assumes`
> > region with the same attributes just like the function inside.
> Your example mixes the global `omp assumes` with the scoped `begin/end omp
> assumes`. The former has the effect to be added to prior declarations, the
> latter does not have this effect. This is how it is supposed to work:
> ```
> void foo(); // no assumption
> #pragma omp begin assumes ext_XYZ
> void bar(); // XYZ assumption
> #pragma omp end assumes
> ```
> ```
> void foo(); // XYZ assumption
> #pragma omp assumes ext_XYZ
> void bar(); // XYZ assumption
> ```
>
>
>
Ah, I see. `omp assumes` looks weird. :( It may lead to conflicts
PS. What about lambdas? Could add a test that lambdas get annotations too?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91980/new/
https://reviews.llvm.org/D91980
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
