[PATCH] D48298: [ELF] Uniquify --wrap list.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In https://reviews.llvm.org/D48298#1140392, @bd1976llvm wrote:

> In https://reviews.llvm.org/D48298#1136343, @grimar wrote:
>
> > In https://reviews.llvm.org/D48298#1136277, @bd1976llvm wrote:
> >
> > > I would propose adding the ability to retrieve a unique filtered list of arguments.
> > >
> > >   for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
> > >     Symtab->addSymbolWrap<ELFT>(Name);
> > >   
> > >
> > > Relevant for other options: --defsym, --trace-symbol, --undefined etc..
> >
> >
> > I am not sure it is useful for other options we have, but it perhaps can be a nice refactoring for this particular case.
> >
> > For example, I am not sure we should use it for --defsym, as in theory it can have different values: `--defsym=foo=1 ... --defsym=foo=2`.
> >  It is OK to process all of them one by one like we do now. The same applies for --trace-symbol and --undefined - there is no issue to process them one by one.
> >  I would not overcomplicate the current code for these cases until it is proven to be a useful change.
>
>
> For `--defsym=foo=2 --defsym=foo=1 --defsym=foo=1 --defsym=foo=3 ` getAllUniqueArgValues() result would be `foo=2`, `foo=1`, `foo=3`. We wouldn't want the result to be sorted.
>  However, looking at the current use cases of getAllArgValues() it doesn't seem worth adding this at the moment.


Yeah, it is all fine with `foo=2`...`foo=3` case. There is another one possible though. Currently, we handle --defsym as a virtual linker script command.
(So we perform evaluations one by one)
If we would have `--defsym=foo=1 --defsym=foo=foo+1 --defsym=foo+1` then deduplication of `--defsym=foo+1` would change the result I believe.
I am not sure how much case is useful/real though (I think not at all), but I would prefer do no changes here than thinking about what might happen :)

> Let's go for the approach than you suggested (putting the don't wrap twice logic into addSymbolWrap).

Thanks for comments, I committed https://reviews.llvm.org/rL335337!


Repository:
  rL LLVM

https://reviews.llvm.org/D48298