[PATCH] D38760: [LLD] [COFF] Add support for automatically exporting all symbols

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: COFF/Driver.cpp:1214
+                      Args.hasArg(OPT_export_all_symbols))) {
+    std::vector<std::string> ExcludeSymbols = {
+        "_NULL_IMPORT_DESCRIPTOR",
----------------
ruiu wrote:
> mstorsjo wrote:
> > zturner wrote:
> > > ruiu wrote:
> > > > Can you make this a separate function which returns a std::set?
> > > Can you make this a `vector<StringRef>`, but add an additional `_` to the beginning of each item?
> > > 
> > > Then, below you can write:
> > > 
> > > ```
> > > if (Config->Machine != I386) {
> > >   for (auto &Str : ExcludeSymbols) {
> > >     Str = Str.drop_front().substr(0, Str.find('@'));
> > >   }
> > > }
> > > ```
> > > 
> > > saving unnecessary allocations on all code paths.
> > Ok, that should be doable.
> Actually, I prefer a simple std::set because the number of items in the set is really small. I wouldn't avoid string copy at all because it's cost is negligible. I'd put emphasis simplicity over efficiency unless it is a performance-critical part of the linker.
Fair enough, but even in that case I prefer `llvm::StringSet` over `std::set`.  I rarely find a valid use case for `std::set` in normal code anymore when various LLVM data structures are almost always better.


https://reviews.llvm.org/D38760