[PATCH] D51664: [IR] Lazily number instructions for local dominance queries
George Burgess IV via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 12 15:32:56 PDT 2018
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
More information about the llvm-commits
mailing list