[PATCH] D154020: [BOLT] Introduce Rewriter interface

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: bolt/include/bolt/Rewrite/RewriteManager.h:48
+
+  /// Use this static method to access global RewriteManager instance.
+  static RewriteManager *getManager();
----------------
Amir wrote:
> Why a global instance and not a RewriteInstance member variable?
I would also recommend to not make this a singleton. It is (in theory) possible to have multiple `RewriteInstance` objects but having a global `RewriteManager` would make this (more) difficult.


================
Comment at: bolt/include/bolt/Rewrite/RewriterBase.h:23
+/// Base class for updating sections containing metadata.
+class RewriterBase {
+  StringRef Name;
----------------
Is this only supposed to be used for rewriting metadata? If so, maybe this needs a more specific name?

For example, at some point I was considering to rewrite instructions pre-CFG to support RISC-V linker relaxation. Would this interface have been suitable for that? (Let's ignore for now whether that's actually a good idea to do :))


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154020/new/

https://reviews.llvm.org/D154020