[PATCH] D60763: Prototype OpenCL BIFs using Tablegen

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

The overall structure looks very clear now! I just have a couple of nitpicks! Thank you!



================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:11
+//
+// This file contains data structures representing OpenCL builtin functions
+// prototypes. In case of an unresolved function names in OpenCL, Clang will
----------------
-> contains definition of OpenCL builtin functions


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:13
+// prototypes. In case of an unresolved function names in OpenCL, Clang will
+// check the function is not a builtin function.
+//
----------------
-> check whether the function is an OpenCL builtin.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:14
+// check the function is not a builtin function.
+//
+//===----------------------------------------------------------------------===//
----------------
We should say that it doesn't contain all function from the spec  i.e. we skip those that don't have overloads or that are added as Clang Builtins.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:17
+//
+// Builtin functions are instances of the Builtin class.
+// Prototypes are defined as a list of Type classes (or its subclasses).
----------------
Can you explain this comment please? At this point it almost feels like you are adding Clang Builtin here. May be you should refer to definition of OpenCL Builtin in this file.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:18
+// Builtin functions are instances of the Builtin class.
+// Prototypes are defined as a list of Type classes (or its subclasses).
+//
----------------
I am not getting this comment. May be we can just leave it out?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:26
+//                            Base definitions
+// These classes/ definitions are representing simple information.
+//===----------------------------------------------------------------------===//
----------------
I feel this comment is too abstract. May be you want to say something like - definitions of misc basic entities? 


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:69
+//===----------------------------------------------------------------------===//
+//                            Classes
+//===----------------------------------------------------------------------===//
----------------
Btw there are classes above too! May be this could be a section about Classes of OpenCL/C type?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:123
+//===----------------------------------------------------------------------===//
+//                            Multiclass definitions
+//===----------------------------------------------------------------------===//
----------------
-> Multiclass definitions of functions with various number of parameters?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:168
+//===----------------------------------------------------------------------===//
+//                            Definitions
+//===----------------------------------------------------------------------===//
----------------
This is also too abstract. How about - Definitions of builtin functions?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:243
+// ========== Builtin definitions ==========
+// This section defines builtin functions found in the OpenCL 1.2 specification
+
----------------
found in -> from


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:305
+
+// ========== Builtin definitions (Examples) ==========
+
----------------
Why examples? I think at this point we shouldn't have examples as this is not a prototype. Also you seem to be using real function, so just remove the Examples. :)


================
Comment at: clang/lib/Sema/SemaLookup.cpp:679
+// prototypes are added to the LookUpResult.
+// S      : The sema instance
+// LR     : The lookupResult instance
----------------
This does not match Clang documentation format. Can you please align with other functions in this file.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:97
+  // static QualType OCL2Qual(ASTContext &Context, OpenCLType Ty);
+  // TODO: Should not need clang::
+  void EmitQualTypeFinder();
----------------
Should we remove the TODO?


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

https://reviews.llvm.org/D60763