Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Peter Maydell]

On 19 September 2016 at 14:10, Daniel P. Berrange <address@hidden> 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