[PATCH] D49596: [Support] Add a UniqueStringSaver: like StringSaver, but deduplicating.

[Sam McCall via Phabricator via llvm-commits]

sammccall added inline comments.


================
Comment at: lib/Support/StringSaver.cpp:26
+  S = Strings.save(S);
+  if (!Unique.insert(S).second)
+    llvm_unreachable("Failed to insert into unique table - race?");
----------------
hokein wrote:
> While we have nice assertion message here, but in return we pay an extra lookup cost. I think we should avoid it (this function is likely to be used in hot path).
> 
> Maybe just
> 
> ```
> auto It = Unique.insert(S);
> if (It.second) {
>   *It.first = Strings.save(V);
> }
> return *It.first.
> ```
I was all ready to explain why this doesn't work (keys in a map/set are const) but... it does.
I think this means `DenseMap` is unsafe (at least less safe than `unordered_map`) but at least this usage is safe. Left a comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D49596