[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][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur closed this revision.
Meinersbur added a comment.

In D74801#189 , @baziotis wrote:

> 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.


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


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.

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()

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

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


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()

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Seems fine to me.


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#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()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.

lg


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(),
SCCE = llvm::scc_end();
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 

[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()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri 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).";
   }

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


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()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri 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).";
   }

err, i was specifically talking about this



Comment at: llvm/tools/opt/PrintSCC.cpp:104-105
  : "external node") << ", ";
-if (nextSCC.size() == 1 && SCCI.hasLoop())
-  errs() << " (Has self-loop).";
+if (nextSCC.size() == 1 && SCCI.hasCycle())
+  errs() << " (Has self-cycle).";
   }

And this


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 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()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


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()

[Eli Friedman via Phabricator via cfe-commits]

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

Seems fine.


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