https://github.com/steffenlarsen updated https://github.com/llvm/llvm-project/pull/184581
>From 79a8b052435d1b3db04245ba411d90d217cf291c Mon Sep 17 00:00:00 2001 From: Steffen Holst Larsen <[email protected]> Date: Wed, 4 Mar 2026 04:11:40 -0600 Subject: [PATCH 1/4] [LLVM][Support][NFCI] Move default values out of the storage Currently, the default value of an option is stored in the storage of the option. This structure muddies the concept of a storage class, as the default value, like with information stored in the parent classes, isn't intended to move with parsing but rather during initialization of the option. This commit proposes a refactor of the CommandLine library to move the default value out of the storage and into the option itself. This allows for a clearer separation between the storage and the option and hides mutable access to the default value from outside the option class, e.g. by no longer exposing an "initial" argument in the `setValue` member function. Signed-off-by: Steffen Holst Larsen <[email protected]> --- llvm/include/llvm/Support/CommandLine.h | 118 ++++++++------------- llvm/unittests/Support/CommandLineTest.cpp | 16 +-- 2 files changed, 54 insertions(+), 80 deletions(-) diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h index be754f3c159ca..dfa58bed0a788 100644 --- a/llvm/include/llvm/Support/CommandLine.h +++ b/llvm/include/llvm/Support/CommandLine.h @@ -1354,7 +1354,6 @@ template <class Opt, class Mod> void apply(Opt *O, const Mod &M) { template <class DataType, bool ExternalStorage, bool isClass> class opt_storage { DataType *Location = nullptr; // Where to store the object... - OptionValue<DataType> Default; void check_location() const { assert(Location && "cl::location(...) not specified for a command " @@ -1362,22 +1361,20 @@ class opt_storage { "or cl::init specified before cl::location()!!"); } -public: - opt_storage() = default; - +protected: bool setLocation(Option &O, DataType &L) { if (Location) return O.error("cl::location(x) specified more than once!"); Location = &L; - Default = L; return false; } - template <class T> void setValue(const T &V, bool initial = false) { +public: + opt_storage() = default; + + template <class T> void setValue(const T &V) { check_location(); *Location = V; - if (initial) - Default = V; } DataType &getValue() { @@ -1390,8 +1387,6 @@ class opt_storage { } operator DataType() const { return this->getValue(); } - - const OptionValue<DataType> &getDefault() const { return Default; } }; // Define how to hold a class type object, such as a string. Since we can @@ -1401,18 +1396,10 @@ class opt_storage { template <class DataType> class opt_storage<DataType, false, true> : public DataType { public: - OptionValue<DataType> Default; - - template <class T> void setValue(const T &V, bool initial = false) { - DataType::operator=(V); - if (initial) - Default = V; - } + template <class T> void setValue(const T &V) { DataType::operator=(V); } DataType &getValue() { return *this; } const DataType &getValue() const { return *this; } - - const OptionValue<DataType> &getDefault() const { return Default; } }; // Define a partial specialization to handle things we cannot inherit from. In @@ -1422,22 +1409,15 @@ class opt_storage<DataType, false, true> : public DataType { template <class DataType> class opt_storage<DataType, false, false> { public: DataType Value; - OptionValue<DataType> Default; // Make sure we initialize the value with the default constructor for the // type. - opt_storage() : Value(DataType()), Default() {} + opt_storage() : Value(DataType()) {} - template <class T> void setValue(const T &V, bool initial = false) { - Value = V; - if (initial) - Default = V; - } + template <class T> void setValue(const T &V) { Value = V; } DataType &getValue() { return Value; } DataType getValue() const { return Value; } - const OptionValue<DataType> &getDefault() const { return Default; } - operator DataType() const { return getValue(); } // If the datatype is a pointer, support -> on it. @@ -1453,6 +1433,7 @@ class opt : public Option, public opt_storage<DataType, ExternalStorage, std::is_class_v<DataType>> { ParserClass Parser; + OptionValue<DataType> Default; bool handleOccurrence(unsigned pos, StringRef ArgName, StringRef Arg) override { @@ -1485,17 +1466,16 @@ class opt } void printOptionValue(size_t GlobalWidth, bool Force) const override { - if (Force || !this->getDefault().compare(this->getValue())) { - cl::printOptionDiff<ParserClass>(*this, Parser, this->getValue(), - this->getDefault(), GlobalWidth); + if (Force || !Default.compare(this->getValue())) { + cl::printOptionDiff<ParserClass>(*this, Parser, this->getValue(), Default, + GlobalWidth); } } void setDefault() override { if constexpr (std::is_assignable_v<DataType &, DataType>) { - const OptionValue<DataType> &V = this->getDefault(); - if (V.hasValue()) - this->setValue(V.getValue()); + if (Default.hasValue()) + this->setValue(Default.getValue()); else this->setValue(DataType()); } @@ -1512,7 +1492,25 @@ class opt opt &operator=(const opt &) = delete; // setInitialValue - Used by the cl::init modifier... - void setInitialValue(const DataType &V) { this->setValue(V, true); } + void setInitialValue(const DataType &V) { + this->setValue(V); + Default = V; + } + + bool setLocation(Option &O, DataType &L) { + // Only external storage options can have their location set. For these we + // also need to set the default value. + if constexpr (ExternalStorage) { + if (opt_storage<DataType, ExternalStorage, + std::is_class_v<DataType>>::setLocation(O, L)) + return true; + Default = L; + return false; + } + return true; + } + + const OptionValue<DataType> &getDefault() const { return Default; } ParserClass &getParser() { return Parser; } @@ -1565,9 +1563,6 @@ extern template class LLVM_TEMPLATE_ABI opt<bool>; // template <class DataType, class StorageClass> class list_storage { StorageClass *Location = nullptr; // Where to store the object... - std::vector<OptionValue<DataType>> Default = - std::vector<OptionValue<DataType>>(); - bool DefaultAssigned = false; public: list_storage() = default; @@ -1581,22 +1576,12 @@ template <class DataType, class StorageClass> class list_storage { return false; } - template <class T> void addValue(const T &V, bool initial = false) { + template <class T> void addValue(const T &V) { assert(Location != nullptr && "cl::location(...) not specified for a command " "line option with external storage!"); Location->push_back(V); - if (initial) - Default.push_back(V); - } - - const std::vector<OptionValue<DataType>> &getDefault() const { - return Default; } - - void assignDefault() { DefaultAssigned = true; } - void overwriteDefault() { DefaultAssigned = false; } - bool isDefaultAssigned() { return DefaultAssigned; } }; // Define how to hold a class type object, such as a string. @@ -1609,8 +1594,6 @@ template <class DataType, class StorageClass> class list_storage { // template <class DataType> class list_storage<DataType, bool> { std::vector<DataType> Storage; - std::vector<OptionValue<DataType>> Default; - bool DefaultAssigned = false; public: using iterator = typename std::vector<DataType>::iterator; @@ -1674,19 +1657,7 @@ template <class DataType> class list_storage<DataType, bool> { std::vector<DataType> *operator&() { return &Storage; } const std::vector<DataType> *operator&() const { return &Storage; } - template <class T> void addValue(const T &V, bool initial = false) { - Storage.push_back(V); - if (initial) - Default.push_back(OptionValue<DataType>(V)); - } - - const std::vector<OptionValue<DataType>> &getDefault() const { - return Default; - } - - void assignDefault() { DefaultAssigned = true; } - void overwriteDefault() { DefaultAssigned = false; } - bool isDefaultAssigned() { return DefaultAssigned; } + template <class T> void addValue(const T &V) { Storage.push_back(V); } }; //===----------------------------------------------------------------------===// @@ -1697,6 +1668,8 @@ template <class DataType, class StorageClass = bool, class list : public Option, public list_storage<DataType, StorageClass> { std::vector<unsigned> Positions; ParserClass Parser; + std::vector<OptionValue<DataType>> Default; + bool DefaultAssigned = false; enum ValueExpected getValueExpectedFlagDefault() const override { return Parser.getValueExpectedFlagDefault(); @@ -1710,9 +1683,9 @@ class list : public Option, public list_storage<DataType, StorageClass> { StringRef Arg) override { typename ParserClass::parser_data_type Val = typename ParserClass::parser_data_type(); - if (list_storage<DataType, StorageClass>::isDefaultAssigned()) { + if (DefaultAssigned) { clear(); - list_storage<DataType, StorageClass>::overwriteDefault(); + DefaultAssigned = false; } if (Parser.parse(*this, ArgName, Arg, Val)) return true; // Parse Error! @@ -1740,7 +1713,7 @@ class list : public Option, public list_storage<DataType, StorageClass> { void setDefault() override { Positions.clear(); list_storage<DataType, StorageClass>::clear(); - for (auto &Val : list_storage<DataType, StorageClass>::getDefault()) + for (auto &Val : Default) list_storage<DataType, StorageClass>::addValue(Val.getValue()); } @@ -1768,11 +1741,12 @@ class list : public Option, public list_storage<DataType, StorageClass> { // setInitialValues - Used by the cl::list_init modifier... void setInitialValues(ArrayRef<DataType> Vs) { - assert(!(list_storage<DataType, StorageClass>::isDefaultAssigned()) && - "Cannot have two default values"); - list_storage<DataType, StorageClass>::assignDefault(); - for (auto &Val : Vs) - list_storage<DataType, StorageClass>::addValue(Val, true); + assert(!DefaultAssigned && "Cannot have two default values"); + DefaultAssigned = true; + for (auto &Val : Vs) { + list_storage<DataType, StorageClass>::addValue(Val); + Default.push_back(OptionValue<DataType>(Val)); + } } void setNumAdditionalVals(unsigned n) { Option::setNumAdditionalVals(n); } diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index 9e8a165fe136f..60e582a79b3eb 100644 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -2191,19 +2191,19 @@ TEST(CommandLineTest, DefaultValue) { EXPECT_TRUE(OS.str().empty()); EXPECT_TRUE(!BoolOption); - EXPECT_FALSE(BoolOption.Default.hasValue()); + EXPECT_FALSE(BoolOption.getDefault().hasValue()); EXPECT_EQ(0, BoolOption.getNumOccurrences()); EXPECT_EQ("", StrOption); - EXPECT_FALSE(StrOption.Default.hasValue()); + EXPECT_FALSE(StrOption.getDefault().hasValue()); EXPECT_EQ(0, StrOption.getNumOccurrences()); EXPECT_TRUE(BoolInitOption); - EXPECT_TRUE(BoolInitOption.Default.hasValue()); + EXPECT_TRUE(BoolInitOption.getDefault().hasValue()); EXPECT_EQ(0, BoolInitOption.getNumOccurrences()); EXPECT_EQ("str-default-value", StrInitOption); - EXPECT_TRUE(StrInitOption.Default.hasValue()); + EXPECT_TRUE(StrInitOption.getDefault().hasValue()); EXPECT_EQ(0, StrInitOption.getNumOccurrences()); const char *Args2[] = {"prog", "-bool-option", "-str-option=str-value", @@ -2214,19 +2214,19 @@ TEST(CommandLineTest, DefaultValue) { EXPECT_TRUE(OS.str().empty()); EXPECT_TRUE(BoolOption); - EXPECT_FALSE(BoolOption.Default.hasValue()); + EXPECT_FALSE(BoolOption.getDefault().hasValue()); EXPECT_EQ(1, BoolOption.getNumOccurrences()); EXPECT_EQ("str-value", StrOption); - EXPECT_FALSE(StrOption.Default.hasValue()); + EXPECT_FALSE(StrOption.getDefault().hasValue()); EXPECT_EQ(1, StrOption.getNumOccurrences()); EXPECT_FALSE(BoolInitOption); - EXPECT_TRUE(BoolInitOption.Default.hasValue()); + EXPECT_TRUE(BoolInitOption.getDefault().hasValue()); EXPECT_EQ(1, BoolInitOption.getNumOccurrences()); EXPECT_EQ("str-init-value", StrInitOption); - EXPECT_TRUE(StrInitOption.Default.hasValue()); + EXPECT_TRUE(StrInitOption.getDefault().hasValue()); EXPECT_EQ(1, StrInitOption.getNumOccurrences()); } >From 28bbc5771bf294610f3971e2b71da1d7f18e0d05 Mon Sep 17 00:00:00 2001 From: Steffen Holst Larsen <[email protected]> Date: Wed, 4 Mar 2026 05:31:39 -0600 Subject: [PATCH 2/4] Fix mlir use-case Signed-off-by: Steffen Holst Larsen <[email protected]> --- llvm/include/llvm/Support/CommandLine.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h index dfa58bed0a788..e0a270c69896a 100644 --- a/llvm/include/llvm/Support/CommandLine.h +++ b/llvm/include/llvm/Support/CommandLine.h @@ -1722,6 +1722,11 @@ class list : public Option, public list_storage<DataType, StorageClass> { Parser.initialize(); } +protected: + bool isDefaultAssigned() const { return DefaultAssigned; } + void overwriteDefault() { DefaultAssigned = false; } + ArrayRef<OptionValue<DataType>> getDefault() const { return Default; } + public: // Command line options should not be copyable list(const list &) = delete; >From bd9f4fae09d4af997c6cb0fc77eab3059d4acb9a Mon Sep 17 00:00:00 2001 From: Steffen Holst Larsen <[email protected]> Date: Wed, 4 Mar 2026 06:52:57 -0600 Subject: [PATCH 3/4] Avoid the preservation of parsed options as default in clangd Signed-off-by: Steffen Holst Larsen <[email protected]> --- clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp b/clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp index d3eb0e152cd0d..e60496c32ef24 100644 --- a/clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp +++ b/clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp @@ -418,11 +418,17 @@ int main(int argc, const char *argv[]) { // Preserve global options when flag parser is reset, so commands can use // them. - IndexLocation.setValue(IndexLocation, /*initial=*/true); - ExecCommand.setValue(ExecCommand, /*initial=*/true); - ProjectRoot.setValue(ProjectRoot, /*initial=*/true); + auto PreservedIndexLocation = IndexLocation.getValue(); + auto PreservedExecCommand = ExecCommand.getValue(); + auto PreservedProjectRoot = ProjectRoot.getValue(); llvm::cl::ResetCommandLineParser(); // We reuse it for REPL commands. + + // Restore preserved options. + IndexLocation.setValue(PreservedIndexLocation); + ExecCommand.setValue(PreservedExecCommand); + ProjectRoot.setValue(PreservedProjectRoot); + llvm::sys::PrintStackTraceOnErrorSignal(argv[0]); bool RemoteMode = llvm::StringRef(IndexLocation).starts_with("remote:"); >From 83e7870bd533d6f714f82a096f1632ef9711748e Mon Sep 17 00:00:00 2001 From: Steffen Holst Larsen <[email protected]> Date: Thu, 5 Mar 2026 01:51:30 -0600 Subject: [PATCH 4/4] Address comments Signed-off-by: Steffen Holst Larsen <[email protected]> --- llvm/include/llvm/Support/CommandLine.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h index e0a270c69896a..580cd03e97122 100644 --- a/llvm/include/llvm/Support/CommandLine.h +++ b/llvm/include/llvm/Support/CommandLine.h @@ -1501,11 +1501,11 @@ class opt // Only external storage options can have their location set. For these we // also need to set the default value. if constexpr (ExternalStorage) { - if (opt_storage<DataType, ExternalStorage, - std::is_class_v<DataType>>::setLocation(O, L)) - return true; - Default = L; - return false; + if (!opt_storage<DataType, ExternalStorage, + std::is_class_v<DataType>>::setLocation(O, L)) { + Default = L; + return false; + } } return true; } @@ -1723,6 +1723,7 @@ class list : public Option, public list_storage<DataType, StorageClass> { } protected: + // MLIR pass option classes uses these to manage default values. bool isDefaultAssigned() const { return DefaultAssigned; } void overwriteDefault() { DefaultAssigned = false; } ArrayRef<OptionValue<DataType>> getDefault() const { return Default; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
