[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:151
+
+  virtual void anchor() override;
+};
----------------
The `virtual` seems unnecessary here, since we have `override`.  I did a quick grep of llvm/include/llvm and other headers seem to agree with me.


================
Comment at: llvm/lib/Support/Anonymization.cpp:54
+StringRef IncrementAnonymizer::anonymizeImpl(StringRef S, bool Reverse,
+                                             bool KeepPrefixfix) {
+  SmallString<128> NextVal;
----------------
Another "fixfix" typo here.


================
Comment at: llvm/lib/Support/Anonymization.cpp:59
+  if (Reverse) {
+    // If symbol begins with l or L, keep that prefix
+    if (KeepPrefixfix && (S.startswith("L") || S.startswith("l")))
----------------
Missing period at the end of the sentence.  Also, I think "If S" would be more clear (but if you disagree, please make it "If the symbol").


================
Comment at: llvm/lib/Support/Anonymization.cpp:68-71
+  if (Reverse) {
+    ReverseMap.push_back(copyString(S));
+    assert(Num == ReverseMap.size() && "ReverseMap has wrong size");
+  }
----------------
Is there a reason this isn't inside the previous `if` statement?  I wonder if the assertion would be obvious enough to skip in that case.


================
Comment at: llvm/lib/Support/Anonymization.cpp:69
+  if (Reverse) {
+    ReverseMap.push_back(copyString(S));
+    assert(Num == ReverseMap.size() && "ReverseMap has wrong size");
----------------
It's not obvious why it's reasonable to copy `S` here, rather than uniquing in a `StringSet`.  Can you add a comment explaining why this won't allocate too many strings?


================
Comment at: llvm/lib/Support/Anonymization.cpp:96
+
+Error IncrementAnonymizer::readReverseMapImpl(llvm::MemoryBuffer *Buffer) {
+  llvm::StringRef Data(Buffer->getBufferStart(), Buffer->getBufferSize());
----------------
It looks like this could be `const MemoryBuffer &`, since `Buffer` isn't modified, and you assume it isn't `nullptr`.  Is there something I'm missing from the other (yet-to-be-upstreamed) derived class?

Also a nit: it seems like the `llvm::` qualifier is unnecessary.


================
Comment at: llvm/lib/Support/Anonymization.cpp:138
+  const size_t IndexBegin = PrefixIdx + Prefix.size();
+  const size_t IndexLengt = SuffixIdx - IndexBegin;
+
----------------
Should this be "IndexLength"?

More generally, should this really be:
```
StringRef Index = Key.slice(...);
```
?


================
Comment at: llvm/unittests/Support/AnonymizationTest.cpp:207
+  auto E = Anonymizer.readReverseMap(Buffer.get());
+  EXPECT_TRUE(E.operator bool());
+  llvm::consumeError(std::move(E));
----------------
The `.operator bool()` seems funny.  Does `EXPECT_TRUE(E)` not work?  If not, I think `EXPECT_TRUE(bool(E))` would be clearer than what's currently there.


================
Comment at: llvm/unittests/Support/AnonymizationTest.cpp:211
+
+TEST(AnonymizationTest, deserializationNoVersion) {
+  std::string Serialized = "foo\nbar\nbaz\n";
----------------
It might be nice to add a comment saying that this is checking for version "1.0".


https://reviews.llvm.org/D45930