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.