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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:20
+//
+// Note: The DEBUG_TYPE macro should be defined before using this
+// file so that any use of LLVM_DEBUG is associated with the
----------------
sameerds wrote:
> foad wrote:
> > Not sure what this comment is for. There aren't any uses of LLVM_DEBUG in this file. And if there were, and you forgot to define DEBUG_TYPE first, you'd get a compiler error wouldn't you?
> I am not sure what the objection is. This comment simply explains what to do if there is an LLVM_DEBUG occurrence. This file specifically does not define DEBUG_TYPE so that the messages can be enabled by the file that actually specializes these templates. It so happens that this particular file does not contain any LLVM_DEBUG messages, but I am  using this as a standard header for more such files in the pipeline.
No strong objection, it just seems like the kind of comment you could put in any header file in the whole project, which suggests to me that it's not adding much value.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:225
+  const_child_iterator child_begin() const {
+    return const_child_iterator{Children.begin()};
+  }
----------------
sameerds wrote:
> foad wrote:
> > Needs whitespace inside braces (doesn't clang-format do this?).
> Those braces enclose an initializer list and not a block of statements. Hence no space inside them.
Good point! I'm still not used to braced initializer lists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696