[PATCH] D68056: [Polly][NFC][ScopBuilder] Move RecordedAssumptions vector to ScopBuilder.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Could you upload the nest diff with context (`git diff -U999999`)?



================
Comment at: polly/include/polly/ScopBuilder.h:74
+  ///      only be simplified later on.
+  RecordedAssumptionsTy RecordedAssumptions;
+
----------------
[style] `RecordedAssumptionsTy RecordedAssumptions = nullptr` preferable over initializer in the ctor.


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:3230
+    // they are used later by scop.
+    for (auto &size : Access->Sizes) {
+      if (!size)
----------------
[style] No [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Almost Always Auto ]]: for (const SCEV *Size: Access->Sizes)

[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Variable start with upper cases ]].


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:3241
+    // they are used later by scop.
+    for (auto &subscript : Access->Subscripts) {
+      if (!Access->isAffine() || !subscript)
----------------
[style] No [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Almost Always Auto ]]: `for (const SCEV *Subscript : Access->Subscripts)`

[suggestion] Access using a getter might be preferable


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:3799
     LLVM_DEBUG(dbgs() << "SCoP detected but dismissed\n");
+    RecordedAssumptions.clear();
     scop.reset();
----------------
[suggestion] Clearing may not be necessary.


================
Comment at: polly/lib/Support/SCEVAffinator.cpp:120
   this->BB = BB;
+  this->RecordedAssumptions = RecordedAssumptions;
 
----------------
[serious] Assigning the field in one of the methods instead of in the ctor makes the lifetime issue even worse. Is there a reason not to pass `RecordedAssumptions` by function arguments?


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

https://reviews.llvm.org/D68056