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