chapter_05

[ivanych]

What is the line algo = SvIV( ST( i + 1 ) ); needed for?

In the appropriate file Chromaprint.xs this line is missing.

Is that a mistake?

[paultcochrane]

I believe you're correct: the algo = SvIV( ST( i + 1 ) ); line seems to be missing from the Chromaprint.xs file.

Incidentally, I think the current code (in the POD):

// we only store unless it's the algorithm
// then we simply override the value and store it later
if ( strcmp( SvPV_nolen( ST(i) ), "algorithm" ) == 0 ) {
    const char *algo_v = SvPV_nolen(value);
    // check algorithm options
    if (!strcmp( algo_v, "test1")) {
        algo = CHROMAPRINT_ALGORITHM_TEST1;
    } else if (!strcmp(algo_v, "test2")) {
        algo = CHROMAPRINT_ALGORITHM_TEST2;
    } else if (!strcmp(algo_v, "test3")) {
        algo = CHROMAPRINT_ALGORITHM_TEST3;
    } else if (!strcmp(algo_v, "test4")) {
        algo = CHROMAPRINT_ALGORITHM_TEST4;
    } else {
        warn("WARNING: unknown algorithm, using the default");
    }
    algo = SvIV( ST( i + 1 ) );
}

should be changed to this:

// we only store unless it's the algorithm
// then we simply override the value and store it later
if ( strcmp( SvPV_nolen( ST(i) ), "algorithm" ) == 0 ) {
    const char *algo_v = SvPV_nolen(value);
    // check algorithm options
    if (!strcmp( algo_v, "test1")) {
        algo = CHROMAPRINT_ALGORITHM_TEST1;
    } else if (!strcmp(algo_v, "test2")) {
        algo = CHROMAPRINT_ALGORITHM_TEST2;
    } else if (!strcmp(algo_v, "test3")) {
        algo = CHROMAPRINT_ALGORITHM_TEST3;
    } else if (!strcmp(algo_v, "test4")) {
        algo = CHROMAPRINT_ALGORITHM_TEST4;
    } else {
        warn("WARNING: unknown algorithm, using the default");
        algo = SvIV( ST( i + 1 ) );
    }
}

and that the code in Chromaprint.xs should be updated to match.

@xsawyerx: does that look right to you?

[xsawyerx]

@ivanych You're right. That line should not be in there. It was used originally to set the value of the algorithm before I wrote the code that validates what the algorithm is. It should be removed.

@paultcochrane The change you suggest keeps that line, which we agree should be removed, and removes another line, the else clause that stores whatever else the parameter and its value is, which I think is by mistake. Is it by mistake?

[xsawyerx]

I created the following Pull Request for it: #30.

[paultcochrane]

@xsawyerx It seems I misunderstood @ivanych's comment: I thought it was meant that the line was missing from the code and that the text was correct. My apologies for getting it around the wrong way!

@xsawyerx my proposed change only moved the algo line into the else clause of the inner if statement. It doesn't remove the following else clause; I merely didn't mention the second else clause in my quoted text above. That certainly could have been clearer, I admit... Also: thanks for the clarification in the PR!

@ivanych: does the change in #30 fix the issue for you?

[xsawyerx]

I consider this resolved. If not, please reopen.

[ivanych]

Yes, now everything is ok. Thank you!