[PATCH] D89184: Support complex target features combinations

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

Hi Peng,

Looks interesting, but I think the language support needs some more comments about what's expected and in addition through everything else please?

I've added some inline comments with places I think could use it for sure.

-eric



================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2389
     }
-    if (!hasRequiredFeatures(ReqFeatures, CGM, FD, MissingFeature))
+    if (!llvm::all_of(ReqFeatures, [&](StringRef Feature) {
+      if (!CallerFeatureMap.lookup(Feature)) {
----------------
Not sure why the change here. It'd be good to be able to reuse the same code here. What's up?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4683
 
+class TargetFeatures {
+  struct FeatureListStatus {
----------------
Needs comments including an explanation of syntax.


================
Comment at: clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp:6
+
+TEST(CheckTargetFeaturesTest, checkBuiltinFeatures) {
+  auto doCheck = [](StringRef BuiltinFeatures, StringRef FuncFeatures) {
----------------
Comment the test with what you're testing.


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

https://reviews.llvm.org/D89184