[PATCH] D120262: [OpenCL] Handle TypeExtensions in OpenCLBuiltinFileEmitter

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh marked 2 inline comments as done.
svenvh added inline comments.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1183
+      SmallVector<StringRef, 2> ExtVec;
+      TypeExt.split(ExtVec, " ");
+      for (const auto Ext : ExtVec) {
----------------
arkangath wrote:
> Just in case if relevant, your "KeepEmpty" will default to true here.
> I don't know if it is possible or not (not enough context for me), but could the .td file have "Extension0  Extention1" (two spaces) that could lead to one empty StringRef here?
That would lead to generation of `#if defined(Extension0) && defined()`, which would cause a compilation error.  I think that's not unreasonable behavior to keep, to enforce that extensions are separated by exactly one space in the .td file for the sake of consistency.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1191
+  // Emit the #if when at least one extension is required.
+  if (!ExtSet.empty()) {
+    OS << "#if ";
----------------
arkangath wrote:
> Arguably it _may_ be better to early-return here instead of increasing the indentation, but I'm not asking for a change; just suggesting.
> The assigned values into OptionalEndif could be just returned directly rather than storing in a local variable, unless you're predicting further changes to this function benefit from this temporary existing.
You are right, that would align better with the style of `emitExtensionGuard` too; I'll apply your suggestion prior to committing.


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

https://reviews.llvm.org/D120262