[llvm] [TableGen] Allow emitter callbacks to use `const RecordKeeper &` (PR #104716)

[Pierre van Houtryve via llvm-commits]

" + sys::path::filename(Record.getInputFilename()),"
In-Reply-To: <llvm.org/llvm/llvm-project/pull/104716 at github.com>


================
@@ -12,22 +12,76 @@
 
 #include "llvm/TableGen/TableGenBackend.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
+#include <variant>
 
 using namespace llvm;
+using namespace TableGen::Emitter;
 
 const size_t MAX_LINE_LEN = 80U;
 
-namespace llvm::TableGen::Emitter {
-ManagedStatic<cl::opt<FnT>, OptCreatorT> Action;
-void *OptCreatorT::call() {
-  return new cl::opt<FnT>(cl::desc("Action to perform:"));
+using FnT = std::variant<FnNonConstT, FnConstT>;
----------------
Pierre-vh wrote:

Note that this is just a nitpick, not something I'd block the diff over and this is strictly my opinion: Variant is great in some cases, but it's also a complicated class with unclear performance implications. I prefer to avoid it when possible.

In this case, there is another solution that replaces the variant with just a trivial pointer.

> Is that considered safe/kosher per LLVM coding standards?

I don't see why it wouldn't be, I would be concerned for safety if you were casting the non-const version to the const version, then the call site could assume RecordKeeper isn't modified when it is. The other way around is fine -the call site assumes we modify RK when we actually don't, just like in any other function called with a non-const ref

Feel free to wait for other reviewers to weigh in, if someone has a strong preference towards one or the other then just listen to that instead :)

https://github.com/llvm/llvm-project/pull/104716