[PATCH] D100265: [Polly][NFC] Partial refactoring of IslAst and IslAstInfo to use isl++

[Michael Kruse via Phabricator via llvm-commits]

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

> Change the order of attributes in class IslAst to reflect the data dependencies so that the destructor won't complain

Nice observation. Did it crash before changing the order?

Could you move the NFC designation to the end of the title? IMHO `[NFC]` makes the impression that there is component called "NFC". The component here would be IslAst, which you could add as well.
Such as

  [Polly][Ast] Partial refactoring of IslAst and IslAstInfo to use isl++. NFC.

I know that there is not consistent placement of NFC in LLVM commits, just stating my personal preference.

The LLVM coding standard uses auto only in very specific cases <https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>. In the past we made liberal use of auto, but when changing a declaration we can use the occasion to bring it to the latest coding standard.No need to update this patch because of this since it is unrelated with what the patch is doing, but something to consider for future patches.

Would you like me to push the patch to main?



================
Comment at: polly/include/polly/CodeGen/IslAst.h:40
   IslAst &operator=(IslAst &&) = delete;
-  ~IslAst();
 
----------------
Removing custom dtors is always nice.


================
Comment at: polly/lib/CodeGen/IslAst.cpp:409
   if (S.hasTrivialInvalidContext()) {
-    RunCondition = PosCond;
+    RunCondition = std::move(PosCond);
   } else {
----------------
The `isl-noexceptions.h` don't implement move ctors/assignments. so `std::move` currently has no effect.

Still nice to consider move-semantics for when it finally is supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100265