[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Kazu Hirata via Phabricator via cfe-commits]

kazu added a comment.

Thank you for this patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 accepted this revision.
barannikov88 added inline comments.



Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:83
   public:
-ExtraRegInfo() = default;
+ExtraRegInfo() {}
 ExtraRegInfo(const ExtraRegInfo &) = delete;

bkramer wrote:
> barannikov88 wrote:
> > Is it somehow different than ' = default'?
> It makes the class non-trivial, std::optional::emplace has issues with 
> trivial default constructors :(
Ah, right, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer marked an inline comment as done.
bkramer added a comment.

In D140332#4005988 , @MaskRay wrote:

> Can you push `using OptionalFileEntryRef = 
> CustomizableOptional;` and the renaming as a separate commit to 
> make this patch smaller?
> There is a smaller that this rename may be reverted.
> 205c0589f918f95d2f2c586a01bea2716d73d603 
>  
> reverted a change related to `OptionalFileEntryRef`. I assume that your 
> change is fine.

Yeah, I'll split this into smaller pieces once we agree on the direction and 
just submit the alias. I expect a revert or two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer marked an inline comment as done.
bkramer added inline comments.



Comment at: clang/lib/Basic/TargetInfo.cpp:513
   if (Opts.MaxBitIntWidth)
-MaxBitIntWidth = Opts.MaxBitIntWidth;
+MaxBitIntWidth = (unsigned)Opts.MaxBitIntWidth;
 

barannikov88 wrote:
> Nit: C-style casts are generally discouraged (there are several occurrences 
> in this patch).
-> static_cast



Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:83
   public:
-ExtraRegInfo() = default;
+ExtraRegInfo() {}
 ExtraRegInfo(const ExtraRegInfo &) = delete;

barannikov88 wrote:
> Is it somehow different than ' = default'?
It makes the class non-trivial, std::optional::emplace has issues with trivial 
default constructors :(



Comment at: mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp:182
 
-  return ret;
+  return std::move(ret);
 }

barannikov88 wrote:
> clang-tidy would usually complain on this. Does it solve some gcc issue?
It does, this is a bug in GCC 7.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Can you push `using OptionalFileEntryRef = CustomizableOptional;` 
and the renaming as a separate commit to make this patch smaller?
There is a smaller that this rename may be reverted.
205c0589f918f95d2f2c586a01bea2716d73d603 
 reverted 
a change related to `OptionalFileEntryRef`. I assume that your change is fine.




Comment at: mlir/include/mlir/IR/Visitors.h:49
   bool operator==(const WalkResult ) const { return result == rhs.result; }
+  bool operator!=(const WalkResult ) const { return result != rhs.result; }
 

Can be pushed separately


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/Basic/TargetInfo.cpp:513
   if (Opts.MaxBitIntWidth)
-MaxBitIntWidth = Opts.MaxBitIntWidth;
+MaxBitIntWidth = (unsigned)Opts.MaxBitIntWidth;
 

Nit: C-style casts are generally discouraged (there are several occurrences in 
this patch).



Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:83
   public:
-ExtraRegInfo() = default;
+ExtraRegInfo() {}
 ExtraRegInfo(const ExtraRegInfo &) = delete;

Is it somehow different than ' = default'?



Comment at: mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp:182
 
-  return ret;
+  return std::move(ret);
 }

clang-tidy would usually complain on this. Does it solve some gcc issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.
Herald added subscribers: jsetoain, JDevlieghere, ormris.

Worth trying on MSVC, which is the other more likely place to fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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