<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sun, Jul 30, 2017 at 7:14 PM Mandeep Singh Grang via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mgrang added a comment.<br>
<br>
> I still wonder if it'd be better if this was not mutable - a compile time constant, this would also potentially remove the need for all the preprocessor work. It could rely on the reverse iteration being a constant (not a command line arg, or mutable internally) and rely on the compiler doing dead code elimination to remove the unnecessary code instead of #if.<br>
<br>
Removing the #if, would mean having unittest/ADT/ReverseIteration.cpp enabled even for release builds?<br>
Also I don't know why but when I tried removing the #if, there were a lot of unit test crashes/failures. Need to look more closely.<br></blockquote><div><br>Yeah, that seems worth understanding.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Do the #if really harm us (other than looking ugly?).</blockquote><div><br>Complexity/maintenance burden, etc. <br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If we remove them, then from just reading the code it won't be obvious that we are relying on the compiler to optimize away variables/functions?<br></blockquote><div><br>I think that's fine - compile time constant, code'll fold away - but if the supporting classes/functions/etc are problematic to keep around in all builds, maybe we do have to go with the way it is, basically. (I'd still be inclined to make it not configurable, for simplicity - not sure if you've already made that change)<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D35043" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35043</a><br>
<br>
<br>
<br>
</blockquote></div></div>