[PATCH] D82579: [NFC] Extract unifyTargetFeatures

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:93
+    StringRef Name = Features[I];
+    assert(Name[0] == '-' || Name[0] == '+');
+    LastOpt[Name.drop_front(1)] = I;
----------------
I don't think assert should be used for something that may be specified to the user on command line.



================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:97
+
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+    // If this feature was overridden, ignore it.
----------------
Would it be simpler to just iterate over features backwards and skip the features we've already seen?



================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:120
+/// If there are multiple +xxx or -xxx features, keep the last one.
+void unifyTargetFeatures(std::vector<StringRef> &Features);
+
----------------
Returning a new `std::vector<StringRef>` would make it more convenient to use and there's less complexity -- no need for the caller to create a temporary vector and the function itself can just return the temporary vector it collects the items in.

```
for (auto Feature : unifyTargetFeatures(Features)) {
  ...
}
```


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

https://reviews.llvm.org/D82579