Skip to content

[TableGen] Change backend callback to require const RecordKeeper #111064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

jurahul
Copy link
Contributor

@jurahul jurahul commented Oct 3, 2024

Change TableGen backend callback function to require a const RecordKeeper argument (by changing it from function_ref to just a function pointer). This undoes parts of #104716 which was added to enable gradual migration of TableGen backends to use const RecordKeeper (by allowing either const or non-const references). Now that all backends have been migrated to const reference, we do not need this.

This is a part of effort to have better const correctness in TableGen backends:

https://discourse.llvm.org/t/psa-planned-changes-to-tablegen-getallderiveddefinitions-api-potential-downstream-breakages/81089

@jurahul jurahul changed the title [TableGen] Change callback function to require const RecordKeeper [TableGen] Change backend callback to require const RecordKeeper Oct 3, 2024
@jurahul jurahul marked this pull request as ready for review October 4, 2024 01:18
@jurahul jurahul requested a review from arsenm October 4, 2024 01:19
@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2024

@llvm/pr-subscribers-tablegen

Author: Rahul Joshi (jurahul)

Changes

Change TableGen backend callback function to require a const RecordKeeper argument (by changing it from function_ref to just a function pointer). This undoes parts of #104716 which was added to enable gradual migration of TableGen backends to use const RecordKeeper (by allowing either const or non-const references). Now that all backends have been migrated to const reference, we do not need this.

This is a part of effort to have better const correctness in TableGen backends:

https://discourse.llvm.org/t/psa-planned-changes-to-tablegen-getallderiveddefinitions-api-potential-downstream-breakages/81089


Full diff: https://github.com/llvm/llvm-project/pull/111064.diff

2 Files Affected:

  • (modified) llvm/include/llvm/TableGen/TableGenBackend.h (+5-4)
  • (modified) llvm/lib/TableGen/TableGenBackend.cpp (+1-18)
diff --git a/llvm/include/llvm/TableGen/TableGenBackend.h b/llvm/include/llvm/TableGen/TableGenBackend.h
index 80134179850b0e..7a6171c58749fd 100644
--- a/llvm/include/llvm/TableGen/TableGenBackend.h
+++ b/llvm/include/llvm/TableGen/TableGenBackend.h
@@ -23,8 +23,7 @@ class RecordKeeper;
 class raw_ostream;
 
 namespace TableGen::Emitter {
-// Supports const and non-const forms of callback functions.
-using FnT = function_ref<void(RecordKeeper &Records, raw_ostream &OS)>;
+using FnT = void (*)(const RecordKeeper &Records, raw_ostream &OS);
 
 /// Creating an `Opt` object registers the command line option \p Name with
 /// TableGen backend and associates the callback \p CB with that option. If
@@ -37,7 +36,9 @@ struct Opt {
 /// Convienence wrapper around `Opt` that registers `EmitterClass::run` as the
 /// callback.
 template <class EmitterC> class OptClass : Opt {
-  static void run(RecordKeeper &RK, raw_ostream &OS) { EmitterC(RK).run(OS); }
+  static void run(const RecordKeeper &RK, raw_ostream &OS) {
+    EmitterC(RK).run(OS);
+  }
 
 public:
   OptClass(StringRef Name, StringRef Desc) : Opt(Name, run, Desc) {}
@@ -45,7 +46,7 @@ template <class EmitterC> class OptClass : Opt {
 
 /// Apply callback for any command line option registered above. Returns false
 /// is no callback was applied.
-bool ApplyCallback(RecordKeeper &Records, raw_ostream &OS);
+bool ApplyCallback(const RecordKeeper &Records, raw_ostream &OS);
 
 } // namespace TableGen::Emitter
 
diff --git a/llvm/lib/TableGen/TableGenBackend.cpp b/llvm/lib/TableGen/TableGenBackend.cpp
index 210fff65458627..70fccd7671e96d 100644
--- a/llvm/lib/TableGen/TableGenBackend.cpp
+++ b/llvm/lib/TableGen/TableGenBackend.cpp
@@ -25,23 +25,6 @@ using namespace TableGen::Emitter;
 
 const size_t MAX_LINE_LEN = 80U;
 
-// CommandLine options of class type are not directly supported with some
-// specific exceptions like std::string which are safe to copy. In our case,
-// the `FnT` function_ref object is also safe to copy. So provide a
-// specialization of `OptionValue` for `FnT` type that stores it as a copy.
-// This is essentially similar to OptionValue<std::string> specialization for
-// strings.
-template <> struct cl::OptionValue<FnT> final : cl::OptionValueCopy<FnT> {
-  OptionValue() = default;
-
-  OptionValue(const FnT &V) { this->setValue(V); }
-
-  OptionValue<FnT> &operator=(const FnT &V) {
-    setValue(V);
-    return *this;
-  }
-};
-
 namespace {
 struct OptCreatorT {
   static void *call() {
@@ -60,7 +43,7 @@ Opt::Opt(StringRef Name, FnT CB, StringRef Desc, bool ByDefault) {
 
 /// Apply callback specified on the command line. Returns true if no callback
 /// was applied.
-bool llvm::TableGen::Emitter::ApplyCallback(RecordKeeper &Records,
+bool llvm::TableGen::Emitter::ApplyCallback(const RecordKeeper &Records,
                                             raw_ostream &OS) {
   FnT Fn = CallbackFunction->getValue();
   if (!Fn)

Change TableGen backend callback function to require a const
RecordKeeper argument (by changing it from function_ref to just a
function pointer). This undoes parts of
llvm#104716 which was added to
enable gradual migration of TableGen backends to use const RecordKeeper
(by allowing either const or non-const references). Now that all
backends have been migrated to const reference, we do not need this.
@jurahul jurahul force-pushed the tg_callback_const_recordkeeper branch from 00ebf33 to 236b5b4 Compare October 4, 2024 12:53
@jurahul jurahul requested a review from arsenm October 4, 2024 12:53
@jurahul jurahul merged commit b91f0de into llvm:main Oct 4, 2024
9 checks passed
@jurahul jurahul deleted the tg_callback_const_recordkeeper branch October 4, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants