[PATCH] D21894: [ELF] - Fixed incorrect logic of version assignments when mixing wildcards with values matching.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Config.h:40
@@ +39,3 @@
+  llvm::StringRef Name;
+  bool HasWildcard;
+};
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > grimar wrote:
> > > > ruiu wrote:
> > > > > I think it's not worth to store this property as a member of this variable. You can always compute it from `Name` when needed.
> > > > Idea here was to avoid several searches of "*?". Since I do 2 iterations now,
> > > > one for wildcarded names and one for not.
> > > > 
> > > > Also I planned to use this struct to add the flag
> > > > IsExternCpp or something for externs handling patch.
> > > Yes, I understand that, but finding "*?" is pretty cheap, and number of symbols in version scripts is not that large.
> > Ok. 
> > So relative to possible patch for externs:
> > What about possible list of extern c++ symbols ? Do you suggest to make a separate array for them ?
> I didn't suggest that -- `IsExternCpp` may make sense.
Ok. So I am removing HasWildcard and VersionSymbol from this patch, and will probably add VersionSymbol back with IsExternCpp field for externs patch then.

================
Comment at: ELF/SymbolTable.cpp:628
@@ +627,3 @@
+
+  for (Version &V : Reverse(Config->SymbolVersions))
+    for (VersionSymbol &Sym : V.Globals)
----------------
ruiu wrote:
> `Reverse` is overkill. You can just iterate in the reverse order by this.
> 
>   for (size_t I = Config->SymbolVersions.size() - 1; I != 0; --I) {
>     VersionSymbol &Sym = Config->SymbolVersions[I];
>     ...
>   }
Sure I can :) That just looks a bit different from loop above making me think it do something special, when it is not.
Will fix.


http://reviews.llvm.org/D21894