[PATCH] ELFObjectWriter: optimize suffices in strtab

[Hans Wennborg]

> I am in favour of moving this to its own file, by why ADT? All the listed potential users would be using lib/Object, so it can stay there, no?

Yes, lib/Object is of course the right place. Moved it there now.

> LGTM with the class being somewhere in lib/Object.

Thanks! Landing..

> Please cross-reference this from the TODO above StringTableBuilder in tools/yaml2obj/yaml2elf.h

Done. I'll see if I can hook this up in a follow-up patch.

================
Comment at: include/llvm/ADT/StringTableBuilder.h:18-19
@@ +17,4 @@
+namespace llvm {
+
+class StringTableBuilder {
+  SmallString<256> StringTable;
----------------
Sean Silva wrote:
> Sean Silva wrote:
> > Please add a class comment indicating that this is primarily a convenience for doing suffix deduplication.
> also, you probably want to call out the very finicky nature of finalize(), since the behavior of the class changes completely before and after it is called (it would be cleaner and safer to have two separate classes, but I think that might complicate things enough to be a net loss)
Done.

================
Comment at: include/llvm/ADT/StringTableBuilder.h:18-19
@@ +17,4 @@
+namespace llvm {
+
+class StringTableBuilder {
+  SmallString<256> StringTable;
----------------
Hans Wennborg wrote:
> Sean Silva wrote:
> > Sean Silva wrote:
> > > Please add a class comment indicating that this is primarily a convenience for doing suffix deduplication.
> > also, you probably want to call out the very finicky nature of finalize(), since the behavior of the class changes completely before and after it is called (it would be cleaner and safer to have two separate classes, but I think that might complicate things enough to be a net loss)
> Done.
I figured the asserts make it pretty clear what's going on, but I've also expanded the comments a bit.

================
Comment at: lib/Support/StringTableBuilder.cpp:35
@@ +34,3 @@
+
+  StringTable += '\x00';
+  StringRef Previous;
----------------
Rafael Ávila de Espíndola wrote:
> Starting the table with a null character makes this ELF specific? That is fine for now, but please comment.
Done.

================
Comment at: test/MC/ELF/symref.s:116
@@ -115,3 +115,3 @@
 // CHECK-NEXT:   Symbol {
-// CHECK-NEXT:     Name: global1 (66)
+// CHECK-NEXT:     Name: global1 (65)
 // CHECK-NEXT:     Value: 0x14
----------------
Rafael Ávila de Espíndola wrote:
> Just remove the number. The printed name is all that the test needs. These are leftover from the old and horrible elf-dump script.
> 
Done.

http://reviews.llvm.org/D3533