[PATCH] D68064: [Builtins] Provide a mechanism to selectively disable tests based on whether an implementation is provided by a builtin library.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked an inline comment as done.
delcypher added inline comments.


================
Comment at: test/builtins/Unit/lit.cfg.py:108
+    lit_config.fatal('builtins_source_features cannot contain empty features')
+  old_size = len(builtins_source_features_set)
+  builtins_source_features_set.add(builtin_source_feature)
----------------
phosek wrote:
> I'd slightly prefer to do:
> ```
> if builtin_source_feature not in builtins_source_features_set:
>   builtins_source_features_set.add(builtin_source_feature)
> else:
>   builtins_source_feature_duplicates.append(builtin_source_feature)
> ```
> It seems easier to comprehend.
Agreed. That is easier to read. The original code I wrote was written that way is to avoid doing two set look ups, instead the original code only does one.

However this is probably a **very premature optimization** at the expense of readability so let's go with the more readable option for now.




Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D68064