tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
LGTM with a possible nit.
================
Comment at: clang/lib/Basic/BuiltinTargetFeatures.h:32
+/// pairs.
+class TargetFeatures {
+ struct FeatureListStatus {
----------------
yaxunl wrote:
> tra wrote:
> > I don't think it needs to be part of the public API. Feature check function
> > operates on `llvm::StringMap<bool>` and does not need to know about this
> > class, and the implementation of this class can be moved into
> > `Builtins.cpp` where it's actually used.
> It is not a public API since it is under lib/Basic.
>
> The reason it is made a header file because we have a unit test
> clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp for class TargetFeatures.
> This is because TargetFeatures is relatively complicated.
Got it. I've missed the test.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2555-2557
if (FeatureList.empty())
return;
assert(!FeatureList.contains(' ') && "Space in feature list");
----------------
Should we move that into the `evaluateRequiredTargetFeatures` ?
`Empty feature list -> true` sounds like something that belongs to the
evaluation logic. So does the assertion on the no-spaces assumption we make
about the input.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125829/new/
https://reviews.llvm.org/D125829
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
