View Change

1 comment:

• File ich_descriptors.c:

  • Patch Set #5, Line 936:

    		if (content->ISL <= 16)
    			return CHIPSET_5_SERIES_IBEX_PEAK;
    

    I see the same side effects and conclude that for these numbers
    this change does not add assumptions.

    You didn't factor in the addition of a new chipset with a large ISL value. That was the whole point.

    Neither did I factor in the addition of a new chipset with a low ISL value
    or one without an ISL field. I'm not sure if the list of things I didn't
    factor in is finite. Hence, I'm not taking any of it into account. The
    purpose of this patch was not to predict the future.

    
    > I'm going to assume that what you call "the default case" is the
    > return statement on line 943 (after this change). In line 942, we
    > check `content->ISL != 16` always before the default case. So the
    > information you are missing is still there, it's just not in the
    > current state of the code inside an if statement.

    The check you are referring to is for a warning that just tells us that the descriptor was not recognized. It does not enforce anything, unlike the original code which put Ibex Peak detection in an if-statement to enforce proper behavior when Ibex Peak is detected while making the intentions for default behavior obvious.

    Well, beside the "It does not enforce anything" part, you are right.
    You just put it in different words.

    
    > NB. The purpose of the code is to make flashrom work, not to provide
    > a manual for future development.

    Code needs to be maintainable. If it's not obvious how to do future development with this code then that is a problem that we should definitely fix.

    You are assuming that there is a fix. We can't predict the future. We
    can make educated guesses, though. For instance, when I wrote this patch,
    I would not have guessed that a server platform will land on this ICCRIBA
    == 0 branch. Trying to predict such details can lead to walking in circles
    patch wise. For instance, I assume that it's easier to follow the code
    when it's more condensed, like after this patch. Before the entire patch
    train it was a forest of if's and nobody knew where to put anything. We
    should avoid that this happens again.

    Anyway, I already agreed that you should try to go ahead days ago. So I
    don't understand why you have to keep arguing and wasting everybody's
    time.

    
    Unless there is an actual bug that is fixed with this patch we should revert it. The original code is more explicit in each of its codepaths which makes it easier for future developers to avoid problems.

    Only one thing I have to ask you: As the purpose of this patch was to
    normalize, please do so with explicit if's for all chipsets then.
    warn_peculiar_desc() probably shouldn't have to check anything then.

To view, visit change 55651. To unsubscribe, or for help writing mail filters, visit settings.