[PATCH] D85735: [OpenMP] Context selector extensions for template functions

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 5 inline comments as done.
jdoerfert added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:10042
   void ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(
-      FunctionDecl *FD, FunctionDecl *BaseFD);
+      Decl *D, SmallVectorImpl<FunctionDecl *> &Bases);
 
----------------
ABataev wrote:
> JonChesterfield wrote:
> > SmallVectorImpl => SmallVector?
> It is a good practice to pass SmallVectors as SmallVectorImpl to make it independent of concrete SmallVector template specialization.
We usually use the Impl versions for the APIs to avoid hardcoding the "size" everywhere. 


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5826
 
-FunctionDecl *
-Sema::ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(Scope *S,
-                                                                Declarator &D) {
+void Sema::ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
+    Scope *S, Declarator &D, MultiTemplateParamsArg TemplateParamLists,
----------------
JonChesterfield wrote:
> Return SmallVector<FunctionDecl> by value instead of via the reference? Makes it clear that we aren't adding to an existing vector
Hm, I guess we can do that, it will cause some more copies. FWIW, we are adding to an existing vector if bases is not empty. I mean, if you provide an empty vector we add and if there is something in the vector we add.


================
Comment at: clang/test/AST/ast-dump-openmp-begin-declare-variant_template_2.cpp:57
+
+// CHECK:      |-FunctionTemplateDecl [[ADDR_0:0x[a-z0-9]*]] <{{.*}}, line:7:1> line:5:5 also_before
+// CHECK-NEXT: | |-TemplateTypeParmDecl [[ADDR_1:0x[a-z0-9]*]] <line:4:11, col:20> col:20 referenced typename depth 0 index 0 T
----------------
JonChesterfield wrote:
> I'd like a different way for us to test this stuff. Realistically impossible to spot errors in 200 lines of tree. Don't have a suggestion at this time.
I manually checked it, and now we can verify changes. At the end of the day checking this by hand is actually not too bad. The trick is that you can skip most of it. You go to the test function return value at the very end. Each call expression there should call a function that returns 0. So follow the callee and check the return there (nothing else anyway). I used this scheme for all tests and once you did verify one it is really easy to do the others. Assuming my input is proper ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85735/new/

https://reviews.llvm.org/D85735