[PATCH] D51664: [IR] Lazily number instructions for local dominance queries

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

This looks good to me, but like said, I don't have sufficient context to stamp it with great confidence.

Thanks again!



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:433
+  /// assert internally so that we get better location info.
+  bool validateInstrOrdering() const;
+
----------------
nit: I'd expect a function named this to `assert()`. Can we rename it to something that sounds more boolean-y like `isInstrOrderingValid()` (?) and/or add `LLVM_NODISCARD`?


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:433
+  /// assert internally so that we get better location info.
+  bool validateInstrOrdering() const;
+
----------------
george.burgess.iv wrote:
> nit: I'd expect a function named this to `assert()`. Can we rename it to something that sounds more boolean-y like `isInstrOrderingValid()` (?) and/or add `LLVM_NODISCARD`?
nit * 2: I don't know what precedent is set elsewhere, but personally, I feel that if the intent is for this to only be used in NDEBUG builds, we should wrap this function decl in `#ifndef NDEBUG`.

Mostly because I'd prefer the compiler tells me to reconsider my life choices over silently getting potentially-incorrect results. If it turns out that there's a case where the latter is preferable, it's a ~4 line diff. :)


https://reviews.llvm.org/D51664