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

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

This looks good to me. I can't claim to have read the test case in detail. Some style suggestions inline around the vector, but they're not blocking.



================
Comment at: clang/include/clang/Sema/Sema.h:10042
   void ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(
-      FunctionDecl *FD, FunctionDecl *BaseFD);
+      Decl *D, SmallVectorImpl<FunctionDecl *> &Bases);
 
----------------
SmallVectorImpl => SmallVector?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5826
 
-FunctionDecl *
-Sema::ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(Scope *S,
-                                                                Declarator &D) {
+void Sema::ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
+    Scope *S, Declarator &D, MultiTemplateParamsArg TemplateParamLists,
----------------
Return SmallVector<FunctionDecl> by value instead of via the reference? Makes it clear that we aren't adding to an existing vector


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5875
+    // TODO: Verify types for templates eventually.
+    if (!UDeclTy->isDependentType()) {
+      QualType NewType = Context.mergeFunctionTypes(
----------------
JonChesterfield wrote:
> tabs!
This turned out to be how phabricator represents changes in indent. Unfortunate that the symbol is the one I usually see used for tabs.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85735