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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
