[PATCH] D97869: [OpenCL] Add OpenCL builtin test generator

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:969
+
+void OpenCLBuiltinTestEmitter::getTypeLists(
+    Record *Type, TypeFlags &Flags, std::vector<Record *> &TypeList,
----------------
Anastasia wrote:
> I would add a documentation for this function, it is not very obvious.
Not sure what documentation you are expecting?  Are there any particular lines that are "not very obvious"?
Note that there is already a comment for this method up in the class declaration.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1004
+    SmallVectorImpl<SmallVector<std::string, 2>> &Types) {
+  // Find out if there are any GenTypes in this signature, and if so, calculate
+  // into how many signatures they will expand.
----------------
Anastasia wrote:
> Maybe this should be lifted as documentation of the header of this function?
This comment only applies to the first loop-nest.  It documents an implementation detail, so I don't think this comment should be lifted to the method's declaration comment.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1033
+    for (unsigned ArgNum = 0; ArgNum < Signature.size(); ArgNum++) {
+      // For differently-sized GenTypes in a parameter list, the smaller
+      // GenTypes just repeat.
----------------
Anastasia wrote:
> I don't get this comment and its exact reference to the code.
I have expanded the comment, and also factored out an index into a variable.  Hopefully that makes it clear?


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

https://reviews.llvm.org/D97869