| Message ID | 1401400700-3725-2-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tobias, First of all, thanks for spotting those issues and sending patches. However looking at libdrm repo history, your patches don't seem to follow formatting guidelines used there: they lack commit messages (which should say what is changed and why) and your signed-off-by tags. Also it is usually a good idea to include respective maintainers on Cc list, although unfortunately I'm not sure who would that be in case of libdrm. One more comment inline. On 29.05.2014 23:58, Tobias Jakobi wrote: > --- > exynos/fimg2d.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h > index 1aac378..bc45ab5 100644 > --- a/exynos/fimg2d.h > +++ b/exynos/fimg2d.h > @@ -25,7 +25,7 @@ > #define G2D_MAX_CMD_LIST_NR 64 > #define G2D_PLANE_MAX_NR 2 > > -#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d) * 65536.0) > +#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d * 65536.0)) You should also keep the parentheses around d, so that the macro evaluates correctly even if an expression is passed as the argument. Best regards, Tomasz
Hello Tomasz! Tomasz Figa wrote: > However looking at libdrm repo history, your patches don't seem to > follow formatting guidelines used there: they lack commit messages > (which should say what is changed and why) and your signed-off-by tags. Originally I sent these to Rob Clark (with Inki Dae in CC), but he wanted me to use git-send-email to send the patches to dri-devel for review. Which I then did. I don't see how the patches lack commit messages? I don't think any additional explanation is needed to what these changes do. They are one-liners and the issues they address are obvious copy&paste errors (which the fimg2d tests don't discover though). Or am I supposed to name them something like "exynos: fix typo in xyz"? I can resend them with my signed-off if that' fine? I assume adding '--signoff' to git-send-email should do the trick? > Also it is usually a good idea to include respective maintainers on Cc > list, although unfortunately I'm not sure who would that be in case of > libdrm. > > One more comment inline. > > On 29.05.2014 23:58, Tobias Jakobi wrote: >> --- >> exynos/fimg2d.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h >> index 1aac378..bc45ab5 100644 >> --- a/exynos/fimg2d.h >> +++ b/exynos/fimg2d.h >> @@ -25,7 +25,7 @@ >> #define G2D_MAX_CMD_LIST_NR 64 >> #define G2D_PLANE_MAX_NR 2 >> >> -#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d) * 65536.0) >> +#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d * 65536.0)) > You should also keep the parentheses around d, so that the macro > evaluates correctly even if an expression is passed as the argument. I don't think that affects the code using the macro, but you're right of course. I'm going to fix this! > Best regards, > Tomasz With best wishes, Tobias
On 01.06.2014 03:14, Tobias Jakobi wrote: > Hello Tomasz! > > > Tomasz Figa wrote: >> However looking at libdrm repo history, your patches don't seem to >> follow formatting guidelines used there: they lack commit messages >> (which should say what is changed and why) and your signed-off-by tags. > Originally I sent these to Rob Clark (with Inki Dae in CC), but he > wanted me to use git-send-email to send the patches to dri-devel for > review. Which I then did. You went from one extreme to another. ;) Although, I can understand this, because I don't see any clear guidelines for libdrm written anywhere. Linux kernel SubmittingPatches[1] might be a good read to learn certain rules that apply to most open source (or at least mailing list) driven projects. tl;dr: The recommended practice is to send the patches to appropriate mailing lists but also add any potentially interested people on CC. [1] https://www.kernel.org/doc/Documentation/SubmittingPatches > > I don't see how the patches lack commit messages? I don't think any > additional explanation is needed to what these changes do. They are > one-liners and the issues they address are obvious copy&paste errors > (which the fimg2d tests don't discover though). Or am I supposed to name > them something like "exynos: fix typo in xyz"? The subjects are fine, but there should be at least one or two sentences explaining what and why the patch does without looking at the code. For example: Currently the G2D_DOUBLE_TO_FIXED() is defined incorrectly, because ... . This patch fixes the definition by changing ... to ..., so that ... . It is important from maintenance point of view, because it shows the people reading the patch that you know what you do and makes them know too. Also keep in mind repository history, which would be hard to look through using just git log (without -p) if patches weren't described properly. > > I can resend them with my signed-off if that' fine? I assume adding > '--signoff' to git-send-email should do the trick? Should do, assuming that there is such option - I don't see it in git help send-email of my git. I recommend reading section 12 (Sign your work) of [1] to learn everything needed about this tag. > >> Also it is usually a good idea to include respective maintainers on Cc >> list, although unfortunately I'm not sure who would that be in case of >> libdrm. >> >> One more comment inline. >> >> On 29.05.2014 23:58, Tobias Jakobi wrote: >>> --- >>> exynos/fimg2d.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h >>> index 1aac378..bc45ab5 100644 >>> --- a/exynos/fimg2d.h >>> +++ b/exynos/fimg2d.h >>> @@ -25,7 +25,7 @@ >>> #define G2D_MAX_CMD_LIST_NR 64 >>> #define G2D_PLANE_MAX_NR 2 >>> >>> -#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d) * 65536.0) >>> +#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d * 65536.0)) >> You should also keep the parentheses around d, so that the macro >> evaluates correctly even if an expression is passed as the argument. > I don't think that affects the code using the macro, but you're right of > course. I'm going to fix this! Thanks. Best regards, Tomasz
diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h index 1aac378..bc45ab5 100644 --- a/exynos/fimg2d.h +++ b/exynos/fimg2d.h @@ -25,7 +25,7 @@ #define G2D_MAX_CMD_LIST_NR 64 #define G2D_PLANE_MAX_NR 2 -#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d) * 65536.0) +#define G2D_DOUBLE_TO_FIXED(d) ((unsigned int)(d * 65536.0)) enum e_g2d_color_mode { /* COLOR FORMAT */