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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
