[PATCH] D45930: [Support] Upstream anonymization and manipulating of BCSymbolMaps

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:32
+
+/// Base class providing basic anonymization capability.
+class Anonymizer {
----------------
Can you explain how?


================
Comment at: llvm/include/llvm/Support/Anonymization.h:55
+
+  /// Write out the reverse map in textural form.
+  ///
----------------
s/textural/textual/


================
Comment at: llvm/include/llvm/Support/Anonymization.h:107
+  BumpPtrAllocator Alloc;
+  MapTy ForwardMap;
+  MapTy IrreveribleForwardMap;
----------------
It's not really clear what the difference is between the two maps.  When should a derived class use which?  (Can you add comments, either here or in the class comment, explaining?)

It's also a little strange that their names distinguish whether they are "reversible", but I don't see anything in the base class about a reverse map.  Should the reverse map be moved to here?


================
Comment at: llvm/include/llvm/Support/Anonymization.h:108
+  MapTy ForwardMap;
+  MapTy IrreveribleForwardMap;
+
----------------
Should this be `IrreversibleForwardMap`?


================
Comment at: llvm/include/llvm/Support/Anonymization.h:144
+  unsigned Num = 0;
+  unsigned IrNum = 0;
+
----------------
Since "ir" is an acronym, can we make this `IRNum`?  That will match `IRPrefix` as well.


================
Comment at: llvm/lib/Support/Anonymization.cpp:21
+
+StringRef Anonymizer::anonymize(StringRef S, bool Reverse, bool KeepPrefixfix) {
+  // Return if the string is empty.
----------------
s/KeepPrefixfix/KeepPrefix/


================
Comment at: llvm/lib/Support/Anonymization.cpp:28-29
+  if (Reverse) {
+    if (ForwardMap.count(S))
+      return ForwardMap.lookup(S);
+    auto Res = anonymizeImpl(S, Reverse, KeepPrefixfix);
----------------
dexonsmith wrote:
> This code looks identical in structure to the code following for the other map.  Can it be factored out into a private function that takes a `MapTy&`?
It's unfortunate to hit the `StringMap` twice in a row like this.  Is that necessary?


================
Comment at: llvm/lib/Support/Anonymization.cpp:28-32
+    if (ForwardMap.count(S))
+      return ForwardMap.lookup(S);
+    auto Res = anonymizeImpl(S, Reverse, KeepPrefixfix);
+    ForwardMap[S] = Res;
+    return Res;
----------------
This code looks identical in structure to the code following for the other map.  Can it be factored out into a private function that takes a `MapTy&`?


================
Comment at: llvm/lib/Support/Anonymization.cpp:31-32
+    auto Res = anonymizeImpl(S, Reverse, KeepPrefixfix);
+    ForwardMap[S] = Res;
+    return Res;
+  }
----------------
It might be simpler to skip the local variable:
```
return ForwardMap[S] = anonymizeImpl(...)
```


================
Comment at: llvm/lib/Support/Anonymization.cpp:43-45
+  if (ForwardMap.count(Symbol))
+    return ForwardMap.lookup(Symbol);
+
----------------
Hitting the map twice seems unnecessary here.


https://reviews.llvm.org/D45930