[PATCH] D112696: CycleInfo: Introduce cycles as a generalization of loops

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 17 inline comments as done.
sameerds added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:34
+template <typename ContextT>
+bool GenericCycle<ContextT>::contains(const GenericCycle *B) const {
+  if (!B)
----------------
nhaehnle wrote:
> Other code in this file uses the new style function definition syntax, so perhaps do so here as well to be consistent?
I am not sure what you mean here ... this definition passes clang-format and clang-tidy. If you mean the lowercase `::contains`, then that is valid camelBack where the first letter must be lowercase.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:14-15
+///
+/// The term "cycle" is chosen to avoid a clash with the existing notion of
+/// (natural) "loop".
+///
----------------
nhaehnle wrote:
> How about moving everything of this file comment to the new CycleTerminology document (that isn't already there) and then just refer to that document here?
Added appropriate sections to CycleTerminology.rst.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696