[PATCH] D82579: [NFC] Extract unifyTargetFeatures

[Artem Belevich via Phabricator via cfe-commits]

tra marked an inline comment as done.
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;
----------------
yaxunl wrote:
> tra wrote:
> > I don't think assert should be used for something that may be specified to the user on command line.
> > 
> The entries in this array is not directly from command line.
> 
> It is either put in by clang code, or translated from -mxxx or -mno-xxx options whereas the translated entry should always have + or - prefix. So if things go wrong, it is due to developers.
While clang may be the primary user of those options, features can be specified by the user, too.
The general rule of thumb is that invalid external input should produce diagnostics, not compiler crashes.
Asserts are for programming errors. Granted, when the inputs may be  generated by our code the boundary is blurred, but in this case I would err on the side of not crashing.


================
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.
----------------
yaxunl wrote:
> tra wrote:
> > Would it be simpler to just iterate over features backwards and skip the features we've already seen?
> > 
> That may save a little when there are repeating entries but you need to reverse the obtained vector to maintain the order. Since most cases there are just a few unique features, reversing may ends up slower.
Up to you. Performance does not matter here, but simpler code with improved readability is always a plus.


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

https://reviews.llvm.org/D82579