dblaikie added inline comments.
================
Comment at: llvm/include/llvm/ADT/SmallVector.h:905
+
+ const SmallVector &operator=(const std::vector<T> &Vec) {
+ this->assign(Vec.begin(), Vec.end());
----------------
aganea wrote:
> dblaikie wrote:
> > aganea wrote:
> > > rnk wrote:
> > > > +@dblaikie, who knows more about STL container details than I do.
> > > This assignment was added to support converting `FrontendOptions`'s
> > > members from `std::vector` -> `SmallVector` as suggested by @dexonsmith.
> > > As stated above, this was to support code such as:
> > > ```
> > > Opts.ASTMergeFiles = Args.getAllArgValues(OPT_ast_merge);
> > > ```
> > > Which otherwise wouldn't work, as `getAllArgValues()` returns a
> > > `std::vector`.
> > I agree with @dexonsmith's suggestion/agreement with @rnk's suggestion to
> > use SmallVector - but to implement the suggestion I'd rather change this:
> >
> > Opts.ASTMergeFiles = Args.getAllArgValues(OPT_ast_merge);
> >
> > to this:
> >
> > const cl::list &ASTMergeFiles = Args.getAllArgValues(OPT_ast_merge);
> > Opts.ASTMergeFiles.assign(ASTMergeFiles.begin(), ASTMergeFiles.end());
> >
> > As @rnk was mentioning. (alternatively, I suppose, since cl::list already
> > has implicit conversion to std::vector, perhaps adding an implicit
> > conversion to SmallVector /there/ would be consistent/not too bad/better
> > than adding std::vector conversion to SmallVector to SmallVector itself)
> Can we do that in a subsequent patch? This current patch as it stand fixes
> the issue mentioned in the summary.
I'm OK with the narrower fix - changing only the specific container & leaving
the rest. I'd probably suggest putting a comment explaining why this is done.
I'll leave it up to @rnk for the final ack.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69959/new/
https://reviews.llvm.org/D69959
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
