[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D138539#4164313 , @steakhal wrote:

> Oh, I forgot to mention why this change breaks the equality operator of 
> `llvm::StringSet`. It's because the `StringMap::operator==` will try to 
> compare the `value` as well, which is of type `std::nullopt_t` and that has 
> no equality comparison operator.
> Previously, the `enum class NoneType` has a default one.

Since `std::unordered_set` and counterparts like `absl::flat_hash_set` provide 
`operator==`, I think providing `StringMap::operator==` is fine. I don't think 
whether the operation is commonly used, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Balázs Benics via Phabricator via lldb-commits]

steakhal added a comment.

Hm, it would be easier to use `llvm::StringMap` 
instead of `std::nullopt_t`, or some adhoc empty struct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Balázs Benics via Phabricator via lldb-commits]

steakhal added a comment.

Oh, I forgot to mention why this change breaks the equality operator of 
`llvm::StringSet`. It's because the `StringMap::operator==` will try to compare 
the `value` as well, which is of type `std::nullopt_t` and that has no equality 
comparison operator.
Previously, the `enum class NoneType` has a default one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Balázs Benics via Phabricator via lldb-commits]

steakhal added a comment.
Herald added a subscriber: thopre.

This patch breaks `llvm::StringSet` equality.
The following code would no longer compile:

  llvm::StringSet LHS;
  llvm::StringSet RHS;
  bool equal = LHS == RHS;

Such code might be used as gtest assertions like `EXPECT_EQ(LHS, RHS)`.
@kazu Do you think we should address this by providing an equality operator for 
the `StringSet`?
I was thinking of adding this:

  bool operator==(const StringSet ) const {
if (Base::size() != RHS.size())
  return false;
  
// For StringSets we only need to check the keys.
for (const auto  : *this) {
  if (RHS.find(KeyValue.getKey()) == RHS.end())
return false;
}
return true;
  };

Do you think we should backport this to `release/16.x`, given that this could 
break downstream users and that llvm-16.0.0 is not released yet?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Kazu Hirata via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG34bcadc38c22: Use std::nullopt_t instead of NoneType (NFC) 
(authored by kazu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

Files:
  bolt/lib/Profile/DataAggregator.cpp
  bolt/lib/Profile/YAMLProfileWriter.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clangd/support/ThreadsafeFS.h
  clang/include/clang/Analysis/Analyses/PostOrderCFGView.h
  clang/include/clang/Basic/DirectoryEntry.h
  clang/include/clang/Basic/FileEntry.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Tooling/Transformer/Parsing.cpp
  lldb/include/lldb/Utility/Timeout.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  llvm/include/llvm/ADT/ArrayRef.h
  llvm/include/llvm/ADT/Optional.h
  llvm/include/llvm/ADT/StringMapEntry.h
  llvm/include/llvm/ADT/StringSet.h
  llvm/include/llvm/FuzzMutate/OpDescriptor.h
  llvm/include/llvm/Support/SMLoc.h
  llvm/lib/CodeGen/MIRParser/MILexer.cpp
  llvm/lib/Support/Optional.cpp
  mlir/include/mlir/IR/OpDefinition.h
  mlir/include/mlir/IR/OperationSupport.h
  mlir/include/mlir/Support/Timing.h
  mlir/lib/Support/Timing.cpp

Index: mlir/lib/Support/Timing.cpp
===
--- mlir/lib/Support/Timing.cpp
+++ mlir/lib/Support/Timing.cpp
@@ -50,7 +50,7 @@
   llvm::sys::SmartRWMutex identifierMutex;
 
   /// A thread local cache of identifiers to reduce lock contention.
-  ThreadLocalCache *>>
+  ThreadLocalCache *>>
   localIdentifierCache;
 
   TimingManagerImpl() : identifiers(identifierAllocator) {}
Index: mlir/include/mlir/Support/Timing.h
===
--- mlir/include/mlir/Support/Timing.h
+++ mlir/include/mlir/Support/Timing.h
@@ -43,7 +43,7 @@
 /// This is a POD type with pointer size, so it should be passed around by
 /// value. The underlying data is owned by the `TimingManager`.
 class TimingIdentifier {
-  using EntryType = llvm::StringMapEntry;
+  using EntryType = llvm::StringMapEntry;
 
 public:
   TimingIdentifier(const TimingIdentifier &) = default;
Index: mlir/include/mlir/IR/OperationSupport.h
===
--- mlir/include/mlir/IR/OperationSupport.h
+++ mlir/include/mlir/IR/OperationSupport.h
@@ -490,7 +490,7 @@
   using size_type = size_t;
 
   NamedAttrList() : dictionarySorted({}, true) {}
-  NamedAttrList(llvm::NoneType none) : NamedAttrList() {}
+  NamedAttrList(std::nullopt_t none) : NamedAttrList() {}
   NamedAttrList(ArrayRef attributes);
   NamedAttrList(DictionaryAttr attributes);
   NamedAttrList(const_iterator inStart, const_iterator inEnd);
@@ -759,7 +759,7 @@
 class OpPrintingFlags {
 public:
   OpPrintingFlags();
-  OpPrintingFlags(llvm::NoneType) : OpPrintingFlags() {}
+  OpPrintingFlags(std::nullopt_t) : OpPrintingFlags() {}
 
   /// Enables the elision of large elements attributes by printing a lexically
   /// valid but otherwise meaningless form instead of the element data. The
Index: mlir/include/mlir/IR/OpDefinition.h
===
--- mlir/include/mlir/IR/OpDefinition.h
+++ mlir/include/mlir/IR/OpDefinition.h
@@ -41,7 +41,7 @@
   OptionalParseResult(ParseResult result) : impl(result) {}
   OptionalParseResult(const InFlightDiagnostic &)
   : OptionalParseResult(failure()) {}
-  OptionalParseResult(llvm::NoneType) : impl(llvm::None) {}
+  OptionalParseResult(std::nullopt_t) : impl(llvm::None) {}
 
   /// Returns true if we contain a valid ParseResult value.
   bool has_value() const { return impl.has_value(); }
Index: llvm/lib/Support/Optional.cpp
===
--- llvm/lib/Support/Optional.cpp
+++ llvm/lib/Support/Optional.cpp
@@ -9,6 +9,6 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/raw_ostream.h"
 
-llvm::raw_ostream ::operator<<(raw_ostream , NoneType) {
+llvm::raw_ostream ::operator<<(raw_ostream , std::nullopt_t) {
   return OS << "None";
 }
Index: llvm/lib/CodeGen/MIRParser/MILexer.cpp
===
--- llvm/lib/CodeGen/MIRParser/MILexer.cpp
+++ llvm/lib/CodeGen/MIRParser/MILexer.cpp
@@ -33,7 +33,7 @@
   const char *End = nullptr;
 
 public:
-  Cursor(NoneType) {}
+  Cursor(std::nullopt_t) {}
 
   explicit Cursor(StringRef Str) {
 Ptr = Str.data();
Index: llvm/include/llvm/Support/SMLoc.h
===
--- llvm/include/llvm/Support/SMLoc.h
+++ llvm/include/llvm/Support/SMLoc.h
@@ -50,7 +50,7 @@
   SMLoc Start, End;
 
   SMRange() = default;
-  SMRange(NoneType) {}
+  

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:218
+  explicit NameLookup(std::nullopt_t) : Data(nullptr, true) {}
   explicit NameLookup(std::nullptr_t) : Data(nullptr, false) {}
   NameLookup() : NameLookup(nullptr) {}

A maintainer might want to check the uses. Having 3 constructors looks 
confusing to readers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds good to me, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

lldb parts LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[Kazu Hirata via Phabricator via lldb-commits]

kazu created this revision.
Herald added subscribers: Moerafaat, zero9178, bzcheeseman, ayermolo, sdasgup3, 
carlosgalvezp, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, 
msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, 
arpith-jacob, antiagainst, shauheen, rriddle, mehdi_amini, kadircet, arphaman, 
hiraditya.
Herald added a reviewer: rriddle.
Herald added a reviewer: rafauler.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a reviewer: NoQ.
Herald added a reviewer: njames93.
Herald added a project: All.
kazu requested review of this revision.
Herald added subscribers: cfe-commits, llvm-commits, lldb-commits, yota9, 
stephenneuendorffer, nicolasvasilache.
Herald added projects: clang, LLDB, MLIR, LLVM, clang-tools-extra.

This patch replaces those occurrences of NoneType that would trigger
an error if the definition of NoneType were missing in None.h.

To keep this patch focused, I am deliberately not replacing None with
std::nullopt in this patch or updating comments.  They will be
addressed in subsequent patches.

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138539

Files:
  bolt/lib/Profile/DataAggregator.cpp
  bolt/lib/Profile/YAMLProfileWriter.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clangd/support/ThreadsafeFS.h
  clang/include/clang/Analysis/Analyses/PostOrderCFGView.h
  clang/include/clang/Basic/DirectoryEntry.h
  clang/include/clang/Basic/FileEntry.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Tooling/Transformer/Parsing.cpp
  lldb/include/lldb/Utility/Timeout.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  llvm/include/llvm/ADT/ArrayRef.h
  llvm/include/llvm/ADT/Optional.h
  llvm/include/llvm/ADT/StringMapEntry.h
  llvm/include/llvm/ADT/StringSet.h
  llvm/include/llvm/FuzzMutate/OpDescriptor.h
  llvm/include/llvm/Support/SMLoc.h
  llvm/lib/CodeGen/MIRParser/MILexer.cpp
  llvm/lib/Support/Optional.cpp
  mlir/include/mlir/IR/OpDefinition.h
  mlir/include/mlir/IR/OperationSupport.h
  mlir/include/mlir/Support/Timing.h
  mlir/lib/Support/Timing.cpp

Index: mlir/lib/Support/Timing.cpp
===
--- mlir/lib/Support/Timing.cpp
+++ mlir/lib/Support/Timing.cpp
@@ -50,7 +50,7 @@
   llvm::sys::SmartRWMutex identifierMutex;
 
   /// A thread local cache of identifiers to reduce lock contention.
-  ThreadLocalCache *>>
+  ThreadLocalCache *>>
   localIdentifierCache;
 
   TimingManagerImpl() : identifiers(identifierAllocator) {}
Index: mlir/include/mlir/Support/Timing.h
===
--- mlir/include/mlir/Support/Timing.h
+++ mlir/include/mlir/Support/Timing.h
@@ -43,7 +43,7 @@
 /// This is a POD type with pointer size, so it should be passed around by
 /// value. The underlying data is owned by the `TimingManager`.
 class TimingIdentifier {
-  using EntryType = llvm::StringMapEntry;
+  using EntryType = llvm::StringMapEntry;
 
 public:
   TimingIdentifier(const TimingIdentifier &) = default;
Index: mlir/include/mlir/IR/OperationSupport.h
===
--- mlir/include/mlir/IR/OperationSupport.h
+++ mlir/include/mlir/IR/OperationSupport.h
@@ -490,7 +490,7 @@
   using size_type = size_t;
 
   NamedAttrList() : dictionarySorted({}, true) {}
-  NamedAttrList(llvm::NoneType none) : NamedAttrList() {}
+  NamedAttrList(std::nullopt_t none) : NamedAttrList() {}
   NamedAttrList(ArrayRef attributes);
   NamedAttrList(DictionaryAttr attributes);
   NamedAttrList(const_iterator inStart, const_iterator inEnd);
@@ -759,7 +759,7 @@
 class OpPrintingFlags {
 public:
   OpPrintingFlags();
-  OpPrintingFlags(llvm::NoneType) : OpPrintingFlags() {}
+  OpPrintingFlags(std::nullopt_t) : OpPrintingFlags() {}
 
   /// Enables the elision of large elements attributes by printing a lexically
   /// valid but otherwise meaningless form instead of the element data. The
Index: mlir/include/mlir/IR/OpDefinition.h
===
--- mlir/include/mlir/IR/OpDefinition.h
+++ mlir/include/mlir/IR/OpDefinition.h
@@ -41,7 +41,7 @@
   OptionalParseResult(ParseResult result) : impl(result) {}
   OptionalParseResult(const InFlightDiagnostic &)
   : OptionalParseResult(failure()) {}
-  OptionalParseResult(llvm::NoneType) : impl(llvm::None) {}
+  OptionalParseResult(std::nullopt_t) : impl(llvm::None) {}
 
   /// Returns true if we contain a valid ParseResult value.
   bool has_value() const { return impl.has_value(); }
Index: llvm/lib/Support/Optional.cpp