[PATCH] D82579: [NFC] Extract unifyTargetFeatures

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.


================
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:
> > 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.
> user may use -target-feature to directly specify features and they may forget +/- there but that is a -cc1 option, whereas this function is used in `tool` and will never be used in -cc1. The features in this vector can only come from clang driver, where the code is supposed to always add +/- to the entries. How could user inputs can cause +/- not added?
Got it. I indeed had `-target-feature` in mind: https://github.com/llvm/llvm-project/blob/32ea3397bec820f98f0b77ca37244142ce700207/clang/lib/Frontend/CompilerInvocation.cpp#L3648




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

https://reviews.llvm.org/D82579