| Message ID | 20150604163425.GR5176@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jun 04, 2015 at 04:56:18PM +0100, Damien Lespiau wrote: >> I noticed one of those and it turned out we have a few lingering around. > > Yuck. I'd prefer we got the other way. Consider the following diffs for example: What's the, uh, diff between those to consider? Anyway, a quick git grep popularity contest within the kernel tree and checkpatch (WARNING: labels should not be indented) tell me to go with Damien. BR, Jani. > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7f6fd85..342509d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -942,6 +942,7 @@ out: > > pps_unlock(intel_dp); > > + // foo > return ret; > } > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 5901e00..2673347 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -942,6 +942,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > > pps_unlock(intel_dp); > > + // foo > return ret; > } > >> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 4 ++-- >> drivers/gpu/drm/i915/intel_sprite.c | 2 +- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index a96f181..2354927 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6980,7 +6980,7 @@ static int i965gm_get_display_clock_speed(struct drm_device *dev) >> >> return DIV_ROUND_CLOSEST(vco, div_table[cdclk_sel]); >> >> - fail: >> +fail: >> DRM_ERROR("Unable to determine CDCLK. HPLL VCO=%u kHz, CFGC=0x%04x\n", vco, tmp); >> return 200000; >> } >> @@ -7021,7 +7021,7 @@ static int g33_get_display_clock_speed(struct drm_device *dev) >> >> return DIV_ROUND_CLOSEST(vco, div_table[cdclk_sel]); >> >> - fail: >> +fail: >> DRM_ERROR("Unable to determine CDCLK. HPLL VCO=%u kHz, CFGC=0x%08x\n", vco, tmp); >> return 190476; >> } >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index 8193a35..f5965fb 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -1189,6 +1189,6 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) >> >> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); >> >> - out: >> +out: >> return ret; >> } >> -- >> 2.1.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jun 05, 2015 at 12:24:45PM +0300, Jani Nikula wrote: > On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Jun 04, 2015 at 04:56:18PM +0100, Damien Lespiau wrote: > >> I noticed one of those and it turned out we have a few lingering around. > > > > Yuck. I'd prefer we got the other way. Consider the following diffs for example: > > What's the, uh, diff between those to consider? Look at the @@ line. One tells you in which function the line is added, the other one doesn't. It always pisses me off when reviewing patches cause then I have to figure out the function based on the label, surroundng context, and/or line numbers. I'm also thinking this may have caused some of the numerous misapplied patches we've had since our labels all tend to be similar. > > Anyway, a quick git grep popularity contest within the kernel tree and > checkpatch (WARNING: labels should not be indented) tell me to go with > Damien. > > BR, > Jani. > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 7f6fd85..342509d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -942,6 +942,7 @@ out: > > > > pps_unlock(intel_dp); > > > > + // foo > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 5901e00..2673347 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -942,6 +942,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > > > > pps_unlock(intel_dp); > > > > + // foo > > return ret; > > } > > > >> > >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 4 ++-- > >> drivers/gpu/drm/i915/intel_sprite.c | 2 +- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index a96f181..2354927 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -6980,7 +6980,7 @@ static int i965gm_get_display_clock_speed(struct drm_device *dev) > >> > >> return DIV_ROUND_CLOSEST(vco, div_table[cdclk_sel]); > >> > >> - fail: > >> +fail: > >> DRM_ERROR("Unable to determine CDCLK. HPLL VCO=%u kHz, CFGC=0x%04x\n", vco, tmp); > >> return 200000; > >> } > >> @@ -7021,7 +7021,7 @@ static int g33_get_display_clock_speed(struct drm_device *dev) > >> > >> return DIV_ROUND_CLOSEST(vco, div_table[cdclk_sel]); > >> > >> - fail: > >> +fail: > >> DRM_ERROR("Unable to determine CDCLK. HPLL VCO=%u kHz, CFGC=0x%08x\n", vco, tmp); > >> return 190476; > >> } > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > >> index 8193a35..f5965fb 100644 > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >> @@ -1189,6 +1189,6 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > >> > >> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); > >> > >> - out: > >> +out: > >> return ret; > >> } > >> -- > >> 2.1.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On Fri, Jun 05, 2015 at 12:27:21PM +0300, Ville Syrjälä wrote: > On Fri, Jun 05, 2015 at 12:24:45PM +0300, Jani Nikula wrote: > > On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Thu, Jun 04, 2015 at 04:56:18PM +0100, Damien Lespiau wrote: > > >> I noticed one of those and it turned out we have a few lingering around. > > > > > > Yuck. I'd prefer we got the other way. Consider the following diffs for example: > > > > What's the, uh, diff between those to consider? > > Look at the @@ line. One tells you in which function the line is added, > the other one doesn't. It always pisses me off when reviewing patches > cause then I have to figure out the function based on the label, > surroundng context, and/or line numbers. > > I'm also thinking this may have caused some of the numerous misapplied > patches we've had since our labels all tend to be similar. Oh wtf! That sounds like something that should be fixed in the tool, a fun little project for a dark winter night.
On 05/06/15 11:04, Damien Lespiau wrote: > On Fri, Jun 05, 2015 at 12:27:21PM +0300, Ville Syrjälä wrote: >> On Fri, Jun 05, 2015 at 12:24:45PM +0300, Jani Nikula wrote: >>> On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >>>> On Thu, Jun 04, 2015 at 04:56:18PM +0100, Damien Lespiau wrote: >>>>> I noticed one of those and it turned out we have a few lingering around. >>>> >>>> Yuck. I'd prefer we got the other way. Consider the following diffs for example: >>> >>> What's the, uh, diff between those to consider? >> >> Look at the @@ line. One tells you in which function the line is added, >> the other one doesn't. It always pisses me off when reviewing patches >> cause then I have to figure out the function based on the label, >> surroundng context, and/or line numbers. >> >> I'm also thinking this may have caused some of the numerous misapplied >> patches we've had since our labels all tend to be similar. > > Oh wtf! > > That sounds like something that should be fixed in the tool, a fun > little project for a dark winter night. As a quick workaround, consider putting this in a .gitattributes file: *.c diff=cpp This will tell git diff to use the predefined regex for finding function headers in c++ files for all C files as well. It differs from the default C regex in that it tries to exclude visibility class labels ("protected:" etc) and therefore incidentally excludes all labels ;-) Enjoy! .Dave.
On Fri, Jun 05, 2015 at 12:27:21PM +0300, Ville Syrjälä wrote: > On Fri, Jun 05, 2015 at 12:24:45PM +0300, Jani Nikula wrote: > > On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Thu, Jun 04, 2015 at 04:56:18PM +0100, Damien Lespiau wrote: > > >> I noticed one of those and it turned out we have a few lingering around. > > > > > > Yuck. I'd prefer we got the other way. Consider the following diffs for example: > > > > What's the, uh, diff between those to consider? > > Look at the @@ line. One tells you in which function the line is added, > the other one doesn't. It always pisses me off when reviewing patches > cause then I have to figure out the function based on the label, > surroundng context, and/or line numbers. Yeah that's an annoying sucker but I guess just part of the fail. Imo consistency wins this bikeshed ;-) > I'm also thinking this may have caused some of the numerous misapplied > patches we've had since our labels all tend to be similar. Diff doesn't look at the heading after the @@ but only at concept. And when applying with some mismatches that can end up in really surprising places. Chaning how we place labels won't help. -Daniel
On 15/06/15 13:34, Daniel Vetter wrote: > On Fri, Jun 05, 2015 at 12:27:21PM +0300, Ville Syrjälä wrote: >> On Fri, Jun 05, 2015 at 12:24:45PM +0300, Jani Nikula wrote: >>> On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >>>> On Thu, Jun 04, 2015 at 04:56:18PM +0100, Damien Lespiau wrote: >>>>> I noticed one of those and it turned out we have a few lingering around. >>>> >>>> Yuck. I'd prefer we got the other way. Consider the following diffs for example: >>> >>> What's the, uh, diff between those to consider? >> >> Look at the @@ line. One tells you in which function the line is added, >> the other one doesn't. It always pisses me off when reviewing patches >> cause then I have to figure out the function based on the label, >> surroundng context, and/or line numbers. > > Yeah that's an annoying sucker but I guess just part of the fail. Imo > consistency wins this bikeshed ;-) > >> I'm also thinking this may have caused some of the numerous misapplied >> patches we've had since our labels all tend to be similar. > > Diff doesn't look at the heading after the @@ but only at concept. And > when applying with some mismatches that can end up in really surprising > places. Chaning how we place labels won't help. > -Daniel You could vary the label by giving each one some compressed prefix based on the name of the function it's in, a sort of poor man's namespacing ... i915_do_some_stuff() { ... goto dss_exit; ... dss_exit: return ret; } i915_exciting_new_function() { ... goto enf_exit; ... enf_exit: return ret; } .Dave.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7f6fd85..342509d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -942,6 +942,7 @@ out: pps_unlock(intel_dp); + // foo return ret; } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5901e00..2673347 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -942,6 +942,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, pps_unlock(intel_dp); + // foo return ret; }