On 19 September 2016 at 14:10, Daniel P. Berrange <berra...@redhat.com> wrote: > On Mon, Sep 19, 2016 at 02:01:59PM +0100, Peter Maydell wrote: >> But in general I think that >> "#if 0" should be an error because there's not really any >> good reason for it. For instance in this case there's no >> explanation anywhere in the file of why these particular >> test cases are disabled or in what circumstances they might >> ever in future be enabled. If there's a case for the code being >> possibly enabled at compile time locally or in the future then >> #if SOMETHING (like the #ifdef DEBUG checks) with some comment >> explaining the situation; if there isn't then the code doesn't >> need to be there at all. > > The data in the test file is a conversion of test data from > cryptsetup. Some are disabled since we don't support the > particular hash algorithms yet, but I've been enabling more, > as in this patch series. IMHO the '#if 0' is appropriate as > this is a marker for future todo items, and if I had deleted > the code as suggested, then whoever adds the extra algorithms > in the future will have to go and find the original test data, > and do a data conversion of it again which is just a waste of > their time.
That sounds like it should fall under "#if SOMETHING plus a comment about why it's there", then. thanks -- PMM
