chapter_05 about xs-fun HOT 6 CLOSED

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?

from xs-fun.

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

from xs-fun.

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

from xs-fun.

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

from xs-fun.

I consider this resolved. If not, please reopen.

from xs-fun.

Yes, now everything is ok. Thank you!

from xs-fun.