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

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

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

Thanks for working on this!  I've just done a pass through for style, and I have a few nits to pick.

> If there's interest from the community I'd be more than happy to upstream the 
>  anonymization pass as well.

I don't see any reason to hold it back...



================
Comment at: llvm/include/llvm/Support/Anonymization.h:35
+  Anonymizer()
+      : Alloc(), ForwardMap(64, Alloc), IrreveribleForwardMap(32, Alloc) {}
+
----------------
Why is `Alloc()` listed here?


================
Comment at: llvm/include/llvm/Support/Anonymization.h:58
+
+  // Access the allocator.
+  BumpPtrAllocator &getAllocator() { return Alloc; }
----------------
Not sure this comment adds any value.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:61
+
+  // As StringRef::copy, but will terminate with null.
+  char *copyString(StringRef S) {
----------------
Should this have three slashes (`///`) so doxygen understands it?


================
Comment at: llvm/include/llvm/Support/Anonymization.h:68
+
+  // look up symbol in the forward map
+  Expected<StringRef> lookupForwardMap(StringRef Symbol) const;
----------------
Should this be a doxygen comment?  Also, it should likely start witha  capital and end with a period.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:72-78
+  // Derived classes will implement this method to return the newly anonymized
+  // string. Derived classe are free to track internal state, but are not
+  // required to (e.g. the base class tracks the reverse mapping, forward
+  // mapping, and provides an allocator).
+  //
+  // The base class will only ever call this once for any unique input,
+  // enforcing idempotence under sequential composition.
----------------
Should this be a doxygen comment?  Same with comments that follow.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:79
+  // enforcing idempotence under sequential composition.
+  virtual StringRef anonymizeImpl(StringRef, bool, bool) = 0;
+
----------------
I feel like this should have names for the parameters.  It's not obvious from the declaration how this API is used.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:81
+
+  // Other virtual implemationation need to provided by subclass.
+  virtual Expected<StringRef> lookupImpl(StringRef) const = 0;
----------------
Misspelled "implementation"; also, it's not clear to me that this comment adds value.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:82
+  // Other virtual implemationation need to provided by subclass.
+  virtual Expected<StringRef> lookupImpl(StringRef) const = 0;
+  virtual void writeReverseMapImpl(raw_ostream &OS) const = 0;
----------------
Should this have a name for the parameter?


================
Comment at: llvm/include/llvm/Support/Anonymization.h:86-87
+
+  // For sub classes, new up some new memory for a string. Will terminate
+  // The memory with null.
+  char *allocateString(unsigned Length) {
----------------
I think this should be a doxygen comment.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:95
+
+  virtual ~Anonymizer() {}
+
----------------
I think I prefer `= default` to `{}`.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:104
+
+  virtual void anchor() = 0;
+};
----------------
This won't serve as a vtable anchor for `Anonymizer`, because it's pure virtual.  I think you should give it an implementation in the source file.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:118
+  IncrementAnonymizer(StringRef Prefix = "__hidden#", StringRef Suffix = "_",
+                      StringRef IrPrefix = "__ir_hidden#")
+      : Anonymizer(), Pre(copyString(Prefix)), Suf(copyString(Suffix)),
----------------
Since IR is an acronym, this seems better to me as `IRPrefix`.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:119
+                      StringRef IrPrefix = "__ir_hidden#")
+      : Anonymizer(), Pre(copyString(Prefix)), Suf(copyString(Suffix)),
+        IrPre(IrPrefix) {
----------------
I see no reason to list `Anonymizer()` here.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:129
+  virtual void writeReverseMapImpl(raw_ostream &OS) const override;
+  virtual void readReverseMapImpl(MemoryBuffer *buf) override;
+
----------------
Variable name should start with a capital.


================
Comment at: llvm/include/llvm/Support/Anonymization.h:132-136
+  StringRef Pre;
+  StringRef Suf;
+  StringRef IrPre;
+  unsigned Num = 0;
+  unsigned IrNum = 0;
----------------
I would have expected `Prefix` and `Suffix`, since "pre" and "suf" don't seem entirely obvious, and `IRPrefix` and `IRNum`, since "ir" is an acronym.


================
Comment at: llvm/lib/Support/Anonymization.cpp:21
+
+StringRef Anonymizer::anonymize(StringRef Src, bool reverse, bool keepPrefix) {
+  // Return if the string is empty.
----------------
`reverse` and `keepPrefix` should be `Reverse` and `KeepPrefix`.


Repository:
  rL LLVM

https://reviews.llvm.org/D45930