On Tue, Nov 10, 2015 at 11:02:04PM +0100, Bernd Schmidt wrote: > On 11/09/2015 11:53 PM, Trevor Saunders wrote: > >I don't really know him, but I don't really disagree with where he wants > >to get to. However I think we work fairly different ways, and review > >things differently. When I review patches (mostly for stuff more > >directly related to Mozilla my standards are basically it needs to be an > >improvement, and it needs to not introduce bugs. > > What I like to ask for is to not just make mechanical changes, but to apply > a bit of thought while doing so. Often something better can be done with the > same amount of effort (e.g. if one notices that the X_POINTER_IS_Y_POINTER > macros are pointless). Such small oddities in the code add up over time, and > it is better to eliminate them when making a change anyway.
yeah, I kind of have mixed feelings about that, on one hand its stuff that really should be cleaned up. ON the other hand making it non mechanical makes it more work than it would be as a mechanical replacement. I guess it depends how much you value computer time testing vs human time thinking or something. Also "that seems fine, but I see we could clean up X" seems nicer to me than "there's nothing wrong with this, but you might as well do X while your there", but maybe that's just habbit or something silly. > > So I find the it might > >improve things, but it doesn't also accomplish X to berather odd, and > >hard to work with if I think getting directly to X might be hard. > > In cases where getting to X is hard I'd agree, and the EH_RETURN_HANDLER_RTX > thing isn't the best example since it's defined by many ports. What > disappointed me in the previous patch series were cases like > INITIAL_FRAME_ADDRESS_RTX which is used by only one port. In such a case I > don't think it's unreasonable to point out that we actually want to get rid > of these macros rather than going through an intermediate stage. > > Since EH_RETURN_HANDLER_RTX is a slightly different case, and Joseph said he > sees value in the patch, it is OK. Well, I guess if you believe Joseph is right about automating conversion from macros to hooks there's an argument to be made that the intermediate stage is just fine even for macros with one definition, because it'll just be two totally mechanical steps. I'm not totally convinced it'll be easy to automate, but even if its a human making the work mechanical in my mind makes it easier. On the other hand for something like BIGGEST_FIELD_ALIGNMENT and ADJUST_FIELD_ALIGN it seems worth while to take a look at merging those into one hook (of course after the libojbc stuff is sorted). Trev > > > Bernd
