[PATCH] D120262: [OpenCL] Handle TypeExtensions in OpenCLBuiltinFileEmitter
arkangath added inline comments.
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1183
+ SmallVector ExtVec;
+ TypeExt.split(ExtVec, " ");
+ for (const auto Ext : ExtVec) {
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?
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1191
+ // Emit the #if when at least one extension is required.
+ if (!ExtSet.empty()) {
+OS << "#if ";
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120262/new/
https://reviews.llvm.org/D120262
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120262: [OpenCL] Handle TypeExtensions in OpenCLBuiltinFileEmitter
arkangath added inline comments.
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:1183
+ SmallVector ExtVec;
+ TypeExt.split(ExtVec, " ");
+ for (const auto Ext : ExtVec) {
svenvh wrote:
> 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.
Ok, that's fine with me. I just wondered if it'd be easier to just set
KeepEmpty=false on the split() call.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120262/new/
https://reviews.llvm.org/D120262
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
