On Fri, Jun 16, 2017 at 10:27:44AM -0700, Christopher Li wrote: > On Thu, Jun 15, 2017 at 1:20 AM, Luc Van Oostenryck > <luc.vanoostenryck@xxxxxxxxx> wrote: > > > > Please, drop what you have already pulled and take the > > following instead. > > Here is the feed back for "finer control over error vs. warnings" > > > +#define ERROR_CURR_PHASE (1 << 0) > +#define ERROR_PREV_PHASE (1 << 1) > +extern int has_error; > > The current phase vs previous phase define is a bit messy to > maintain when you advance to next phase. Currently your > patch does: > > + > + if (has_error & ERROR_CURR_PHASE) > + has_error = ERROR_PREV_PHASE; > > I purpose each different phase/stage we have a dedicate bits. > #define PHASE_PARSING (1 << 0) > #define PHASE_EVALUATE (1<<1) It's what I did first because it was obvious to do so but then I changed my mind because I found that theer was no real advantages and a few complications too. But I don't really mind, I can change it. .... > That will be gone. The rest of the patch should be very similar. > > This has the advantage of: > - The code clearly show which stage it is in. But what is less clear is in what is that an advantage. > - No need to move error bits from current phase to previous phase. Indeed, one assignment less ... > - more friendly if we have more than two phase. In fact no, you expose something that don't need to. With the one-def-per-phase, it's 'easier' to set the new state (but again, it's just an assignement) but when you need to use what you really need is 'what what the status in the previous phase(s)', whatever the previous state was. But then again, I really don't care. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
