[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a comment.

In D74801#1882155 , @lebedev.ri wrote:

> lg


Great! Just a note: I don't have commit access.


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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a comment.

Ping :)


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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a comment.

In D74801#1897267 , @lattner wrote:

> Seems fine to me.


Thank you! Please note that I don't have commit access.


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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a comment.

In D74801#1898660 , @lattner wrote:

> I'd be happy to help fix that problem.  Please take a look at the llvm 
> developer policy. :-)


Oh thanks, I thought I had to submit way more patches.


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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a comment.

I committed that 
(https://reviews.llvm.org/rG21390eab4c05e0ed7e7d13ada9e85f62b87ea484) but as it 
seems, I should have added the differential division in the commit message. 
I'll try the next commit to be better.


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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a comment.

> If that happens, this Phabricator review does not close automatically. It has 
> to be closed manually using the "Add Action..." dropdown.

Noted.


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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis created this revision.
baziotis added reviewers: dexonsmith, lattner, timshen, chandlerc.
Herald added subscribers: llvm-commits, cfe-commits, arphaman, hiraditya.
Herald added projects: clang, LLVM.

Not a super important change but yesterday I had to look in this function and 
initially it was misleading. I thought it tests if there's a loop that 
satisfies the criteria in the loop terminology 
(https://llvm.org/docs/LoopTerminology.html). Loop is a special term I think so 
maybe the term cycle is better.

P.S.: I hope I added the correct reviewers, which were those I found in the git 
blame.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74801

Files:
  clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
  llvm/include/llvm/ADT/SCCIterator.h
  llvm/lib/IR/ModuleSummaryIndex.cpp
  llvm/tools/opt/PrintSCC.cpp


Index: llvm/tools/opt/PrintSCC.cpp
===
--- llvm/tools/opt/PrintSCC.cpp
+++ llvm/tools/opt/PrintSCC.cpp
@@ -79,8 +79,8 @@
 for (std::vector::const_iterator I = nextSCC.begin(),
E = nextSCC.end(); I != E; ++I)
   errs() << (*I)->getName() << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }
   errs() << "\n";
 
@@ -101,8 +101,8 @@
E = nextSCC.end(); I != E; ++I)
   errs() << ((*I)->getFunction() ? (*I)->getFunction()->getName()
  : "external node") << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }
   errs() << "\n";
 
Index: llvm/lib/IR/ModuleSummaryIndex.cpp
===
--- llvm/lib/IR/ModuleSummaryIndex.cpp
+++ llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -300,7 +300,7 @@
   if (V.getSummaryList().size())
 F = cast(V.getSummaryList().front().get());
   O << " " << (F == nullptr ? "External" : "") << " " << 
utostr(V.getGUID())
-<< (I.hasLoop() ? " (has loop)" : "") << "\n";
+<< (I.hasCycle() ? " (has cycle)" : "") << "\n";
 }
 O << "}\n";
   }
Index: llvm/include/llvm/ADT/SCCIterator.h
===
--- llvm/include/llvm/ADT/SCCIterator.h
+++ llvm/include/llvm/ADT/SCCIterator.h
@@ -124,11 +124,11 @@
 return CurrentSCC;
   }
 
-  /// Test if the current SCC has a loop.
+  /// Test if the current SCC has a cycle.
   ///
   /// If the SCC has more than one node, this is trivially true.  If not, it 
may
-  /// still contain a loop if the node has an edge back to itself.
-  bool hasLoop() const;
+  /// still contain a cycle if the node has an edge back to itself.
+  bool hasCycle() const;
 
   /// This informs the \c scc_iterator that the specified \c Old node
   /// has been deleted, and \c New is to be used in its place.
@@ -212,7 +212,7 @@
 }
 
 template 
-bool scc_iterator::hasLoop() const {
+bool scc_iterator::hasCycle() const {
 assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
 if (CurrentSCC.size() > 1)
   return true;
Index: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
@@ -265,7 +265,7 @@
   for (llvm::scc_iterator SCCI = llvm::scc_begin(&CG),
SCCE = llvm::scc_end(&CG);
SCCI != SCCE; ++SCCI) {
-if (!SCCI.hasLoop()) // We only care about cycles, not standalone nodes.
+if (!SCCI.hasCycle()) // We only care about cycles, not standalone nodes.
   continue;
 handleSCC(*SCCI);
   }


Index: llvm/tools/opt/PrintSCC.cpp
===
--- llvm/tools/opt/PrintSCC.cpp
+++ llvm/tools/opt/PrintSCC.cpp
@@ -79,8 +79,8 @@
 for (std::vector::const_iterator I = nextSCC.begin(),
E = nextSCC.end(); I != E; ++I)
   errs() << (*I)->getName() << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }
   errs() << "\n";
 
@@ -101,8 +101,8 @@
E = nextSCC.end(); I != E; ++I)
   errs() << ((*I)->getFunction() ? (*I)->getFunction()->getName()
  : "external node") << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }
   errs() << "\n";
 
Index: llvm/lib/IR/ModuleSummaryIndex.cpp
===
--- llvm/lib/IR/ModuleSummaryIndex.cp

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis added a subscriber: jdoerfert.
baziotis added a comment.

In D74801#1881679 , @efriedma wrote:

> Seems fine.


Great, thanks and to @jdoerfert!
Any way I can find the correct reviewers next time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

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

In D74801#1881693 , @lebedev.ri wrote:

> While i certainly agree that it's kinda confusing, i'm not really sure this 
> is conceptually correct change.
>  https://en.wikipedia.org/wiki/Loop_(graph_theory)
>
> > In graph theory, a loop (also called a self-loop or a "buckle") is an edge 
> > that connects a vertex to itself. A simple graph contains no loops.
>
> which is exactly the case here.
>
> https://en.wikipedia.org/wiki/Cycle_(graph_theory)
>  seems more generic than that.


Yes, I agree, but it was the closest word I could find that is not "loop" but 
still gets the point across. I mean technically maybe the better term would be 
"hasALoopButNotNecessarilyANormalLoop()" but well... :P
In any case, that was an experience of mine as a newcomer. If you think it's 
not worth it, so be it. :)




Comment at: llvm/tools/opt/PrintSCC.cpp:82-83
   errs() << (*I)->getName() << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }

lebedev.ri wrote:
> err, i was specifically talking about this
Maybe we can keep the "Has self-loop here".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

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



Comment at: llvm/tools/opt/PrintSCC.cpp:82-83
   errs() << (*I)->getName() << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }

lebedev.ri wrote:
> baziotis wrote:
> > lebedev.ri wrote:
> > > err, i was specifically talking about this
> > Maybe we can keep the "Has self-loop here".
> Then this would look good to me
Ok, uploading new diff in a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74801



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

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Stefanos Baziotis via Phabricator via cfe-commits]

baziotis updated this revision to Diff 245295.
baziotis edited the summary of this revision.
baziotis added a comment.

Self-cycle to self-loop


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

https://reviews.llvm.org/D74801

Files:
  clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
  llvm/include/llvm/ADT/SCCIterator.h
  llvm/lib/IR/ModuleSummaryIndex.cpp
  llvm/tools/opt/PrintSCC.cpp


Index: llvm/tools/opt/PrintSCC.cpp
===
--- llvm/tools/opt/PrintSCC.cpp
+++ llvm/tools/opt/PrintSCC.cpp
@@ -79,7 +79,7 @@
 for (std::vector::const_iterator I = nextSCC.begin(),
E = nextSCC.end(); I != E; ++I)
   errs() << (*I)->getName() << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
+if (nextSCC.size() == 1 && SCCI.hasCycle())
   errs() << " (Has self-loop).";
   }
   errs() << "\n";
@@ -101,7 +101,7 @@
E = nextSCC.end(); I != E; ++I)
   errs() << ((*I)->getFunction() ? (*I)->getFunction()->getName()
  : "external node") << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
+if (nextSCC.size() == 1 && SCCI.hasCycle())
   errs() << " (Has self-loop).";
   }
   errs() << "\n";
Index: llvm/lib/IR/ModuleSummaryIndex.cpp
===
--- llvm/lib/IR/ModuleSummaryIndex.cpp
+++ llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -300,7 +300,7 @@
   if (V.getSummaryList().size())
 F = cast(V.getSummaryList().front().get());
   O << " " << (F == nullptr ? "External" : "") << " " << 
utostr(V.getGUID())
-<< (I.hasLoop() ? " (has loop)" : "") << "\n";
+<< (I.hasCycle() ? " (has cycle)" : "") << "\n";
 }
 O << "}\n";
   }
Index: llvm/include/llvm/ADT/SCCIterator.h
===
--- llvm/include/llvm/ADT/SCCIterator.h
+++ llvm/include/llvm/ADT/SCCIterator.h
@@ -124,11 +124,11 @@
 return CurrentSCC;
   }
 
-  /// Test if the current SCC has a loop.
+  /// Test if the current SCC has a cycle.
   ///
   /// If the SCC has more than one node, this is trivially true.  If not, it 
may
-  /// still contain a loop if the node has an edge back to itself.
-  bool hasLoop() const;
+  /// still contain a cycle if the node has an edge back to itself.
+  bool hasCycle() const;
 
   /// This informs the \c scc_iterator that the specified \c Old node
   /// has been deleted, and \c New is to be used in its place.
@@ -212,7 +212,7 @@
 }
 
 template 
-bool scc_iterator::hasLoop() const {
+bool scc_iterator::hasCycle() const {
 assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
 if (CurrentSCC.size() > 1)
   return true;
Index: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
@@ -265,7 +265,7 @@
   for (llvm::scc_iterator SCCI = llvm::scc_begin(&CG),
SCCE = llvm::scc_end(&CG);
SCCI != SCCE; ++SCCI) {
-if (!SCCI.hasLoop()) // We only care about cycles, not standalone nodes.
+if (!SCCI.hasCycle()) // We only care about cycles, not standalone nodes.
   continue;
 handleSCC(*SCCI);
   }


Index: llvm/tools/opt/PrintSCC.cpp
===
--- llvm/tools/opt/PrintSCC.cpp
+++ llvm/tools/opt/PrintSCC.cpp
@@ -79,7 +79,7 @@
 for (std::vector::const_iterator I = nextSCC.begin(),
E = nextSCC.end(); I != E; ++I)
   errs() << (*I)->getName() << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
+if (nextSCC.size() == 1 && SCCI.hasCycle())
   errs() << " (Has self-loop).";
   }
   errs() << "\n";
@@ -101,7 +101,7 @@
E = nextSCC.end(); I != E; ++I)
   errs() << ((*I)->getFunction() ? (*I)->getFunction()->getName()
  : "external node") << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
+if (nextSCC.size() == 1 && SCCI.hasCycle())
   errs() << " (Has self-loop).";
   }
   errs() << "\n";
Index: llvm/lib/IR/ModuleSummaryIndex.cpp
===
--- llvm/lib/IR/ModuleSummaryIndex.cpp
+++ llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -300,7 +300,7 @@
   if (V.getSummaryList().size())
 F = cast(V.getSummaryList().front().get());
   O << " " << (F == nullptr ? "External" : "") << " " << utostr(V.getGUID())
-<< (I.hasLoop() ? " (has loop)" : "") << "\n";
+<< (I.hasCycle() ? " (has cycle)" : "") << "\n";
 }
 O << "}\n";
   }
Index: llvm/include/llvm/ADT/SCCIterator.h
===
--- llvm/include/llvm/ADT/SCCIterator.h
+++ llvm/include/llvm/ADT/SCCIterator.h
@@ -124,11 +124,11 @@
 return Cu