[PATCH] D27716: [ELF] - Implemented --retain-symbols-file option

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Driver.cpp:498-499
+static void parseRetainSymbolsList(MemoryBufferRef MB) {
+  for (StringRef S : getLines(MB))
+    Config->RetainSymbolsFile.insert(S.trim());
+}
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > I think that you are inserting an empty string to the set because usually a text file ends with "\n", so the last line is empty. I'd do trim() and remove blank lines in getLines().
> > No I am not, last argument of split() does all job:
> > 
> > ```
> >   MB.getBuffer().split(Arr, '\n', INT32_MAX, /*KeepEmpty*/ false);
> > ```
> > 
> > Also set would assert that:
> > ```
> >     std::pair<typename base::iterator, bool> insert(StringRef Key) {
> >       assert(!Key.empty());
> >       return base::insert(std::make_pair(Key, '\0'));
> >     }
> > ```
> What if the file ends with "\n " (newline and an space character)?
It would assert, you right. Fixed, thanks !


https://reviews.llvm.org/D27716