[PATCH] D35043: [ADT] Enable reverse iteration for DenseMap

[David Blaikie via llvm-commits]

On Sun, Jul 30, 2017 at 7:14 PM Mandeep Singh Grang via Phabricator <
reviews at reviews.llvm.org> wrote:

> mgrang added a comment.
>
> > 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.
>
> Removing the #if, would mean having unittest/ADT/ReverseIteration.cpp
> enabled even for release builds?
> 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.
>

Yeah, that seems worth understanding.


> Do the #if really harm us (other than looking ugly?).


Complexity/maintenance burden, etc.


> 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?
>

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)

- Dave


>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D35043
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170731/4edd7160/attachment.html>