| Message ID | 1246978928-7139-1-git-send-email-ameya.palande@nokia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Thanks for the patch, it only has indentation problems, this is the checkpatch output:
WARNING: suspect code indent for conditional statements (8, 12)
#34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
+ if (DSP_SUCCEEDED(status)) { \
+ if (unlikely((src) == NULL) || \
WARNING: line over 80 characters
#36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
+ unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \
WARNING: suspect code indent for conditional statements (8, 12)
#46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
+ if (DSP_SUCCEEDED(status)) { \
+ if (unlikely((dest) == NULL) || \
WARNING: line over 80 characters
#48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
+ unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \
total: 0 errors, 4 warnings, 48 lines checked
Could you please fix that warning please?
Regards,
Fernando.
> -----Original Message-----
> From: Ameya Palande [mailto:ameya.palande@nokia.com]
> Sent: Tuesday, July 07, 2009 10:02 AM
> To: linux-omap@vger.kernel.org
> Cc: Guzman Lugo, Fernando; Kanigeri, Hari; ext-phil.2.carmody@nokia.com
> Subject: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an
> if/else
>
> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
>
> cp_{to,fm}_usr break if between an if and an else (with no {}).
>
> http://www.faqs.org/faqs/C-faq/abridged/
> 10.4: What's the best way to write a multi-statement macro?
> A: #define Func() do {stmt1; stmt2; ... } while(0) /* (no trailing ;)
> */
>
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
> drivers/dsp/bridge/pmgr/wcd.c | 42 ++++++++++++++++++++++--------------
> ----
> 1 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
> index 86812c6..aaf3019 100644
> --- a/drivers/dsp/bridge/pmgr/wcd.c
> +++ b/drivers/dsp/bridge/pmgr/wcd.c
> @@ -147,25 +147,29 @@
>
> /* Following two macros should ideally have do{}while(0) */
>
> -#define cp_fm_usr(dest, src, status, elements) \
> - if (DSP_SUCCEEDED(status)) {\
> - if (unlikely(src == NULL) || \
> - unlikely(copy_from_user(dest, src, elements *
> sizeof(*(dest))))) { \
> - GT_1trace(WCD_debugMask, GT_7CLASS, \
> - "copy_from_user failed, src=0x%x\n", src); \
> - status = DSP_EPOINTER ; \
> - } \
> - }
> -
> -#define cp_to_usr(dest, src, status, elements) \
> - if (DSP_SUCCEEDED(status)) {\
> - if (unlikely(dest == NULL) || \
> - unlikely(copy_to_user(dest, src, elements * sizeof(*(src)))))
> { \
> - GT_1trace(WCD_debugMask, GT_7CLASS, \
> - "copy_to_user failed, dest=0x%x\n", dest); \
> - status = DSP_EPOINTER ;\
> - } \
> - }
> +#define cp_fm_usr(dest, src, status, elements) \
> + do { \
> + if (DSP_SUCCEEDED(status)) { \
> + if (unlikely((src) == NULL) || \
> + unlikely(copy_from_user(dest, src, (elements) *
> sizeof(*(dest))))) { \
> + GT_1trace(WCD_debugMask, GT_7CLASS, \
> + "copy_from_user failed, src=0x%x\n", src); \
> + (status) = DSP_EPOINTER ; \
> + } \
> + } \
> + } while (0)
> +
> +#define cp_to_usr(dest, src, status, elements) \
> + do { \
> + if (DSP_SUCCEEDED(status)) { \
> + if (unlikely((dest) == NULL) || \
> + unlikely(copy_to_user(dest, src, (elements) *
> sizeof(*(src))))) { \
> + GT_1trace(WCD_debugMask, GT_7CLASS, \
> + "copy_to_user failed, dest=0x%x\n", dest); \
> + (status) = DSP_EPOINTER ; \
> + } \
> + } \
> + } while (0)
>
> /* Device IOCtl function pointer */
> struct WCD_Cmd {
> --
> 1.6.2.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote: > Thanks for the patch, it only has indentation problems, this is the checkpatch output: > > WARNING: suspect code indent for conditional statements (8, 12) > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152: > + if (DSP_SUCCEEDED(status)) { \ > + if (unlikely((src) == NULL) || \ > > WARNING: line over 80 characters > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154: > + unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \ > > WARNING: suspect code indent for conditional statements (8, 12) > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164: > + if (DSP_SUCCEEDED(status)) { \ > + if (unlikely((dest) == NULL) || \ > > WARNING: line over 80 characters > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166: > + unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \ > > total: 0 errors, 4 warnings, 48 lines checked > > Could you please fix that warning please? I suspect the only way to fix those warnings would either introduce other warnings, or \ would \ lead \ to \ utterly \ unread- \ able \ code. If you check how and why the original TI-originated version of the code does not follow the linux coding standards, the difficulties we would have making a warning-free patch of it should be apparent. Phil -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Phil, > -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@vger.kernel.org] On Behalf Of Phil Carmody > Sent: Tuesday, July 14, 2009 6:03 AM > > On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote: > > Thanks for the patch, it only has indentation problems, this is the > checkpatch output: > > > > WARNING: suspect code indent for conditional statements (8, 12) > > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152: > > + if (DSP_SUCCEEDED(status)) { \ > > + if (unlikely((src) == NULL) || \ > > > > WARNING: line over 80 characters > > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154: > > + unlikely(copy_from_user(dest, src, (elements) * > sizeof(*(dest))))) { \ > > > > WARNING: suspect code indent for conditional statements (8, 12) > > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164: > > + if (DSP_SUCCEEDED(status)) { \ > > + if (unlikely((dest) == NULL) || > \ > > > > WARNING: line over 80 characters > > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166: > > + unlikely(copy_to_user(dest, src, (elements) * > sizeof(*(src))))) { \ > > > > total: 0 errors, 4 warnings, 48 lines checked > > > > Could you please fix that warning please? > > I suspect the only way to fix those warnings would either introduce > other warnings, or \ Gentle query: Have we already tried this or is it just a suspicion? > would \ > lead \ > to \ > utterly \ > unread- \ > able \ > code. > > If you check how and why the original TI-originated version of the code > does not follow the linux coding standards, the difficulties we would > have making a warning-free patch of it should be apparent. Past is past. The idea was not to introduce anymore warning code. Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ext Menon, Nishanth wrote: > Phil, >> -----Original Message----- >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- >> owner@vger.kernel.org] On Behalf Of Phil Carmody >> Sent: Tuesday, July 14, 2009 6:03 AM >> >> On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote: >>> Thanks for the patch, it only has indentation problems, this is the >> checkpatch output: >>> WARNING: suspect code indent for conditional statements (8, 12) >>> #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152: >>> + if (DSP_SUCCEEDED(status)) { \ >>> + if (unlikely((src) == NULL) || \ >>> >>> WARNING: line over 80 characters >>> #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154: >>> + unlikely(copy_from_user(dest, src, (elements) * >> sizeof(*(dest))))) { \ >>> WARNING: suspect code indent for conditional statements (8, 12) >>> #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164: >>> + if (DSP_SUCCEEDED(status)) { \ >>> + if (unlikely((dest) == NULL) || >> \ >>> WARNING: line over 80 characters >>> #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166: >>> + unlikely(copy_to_user(dest, src, (elements) * >> sizeof(*(src))))) { \ >>> total: 0 errors, 4 warnings, 48 lines checked >>> >>> Could you please fix that warning please? >> I suspect the only way to fix those warnings would either introduce >> other warnings, or \ > Gentle query: Have we already tried this or is it just a suspicion? > > >> would \ >> lead \ >> to \ >> utterly \ >> unread- \ >> able \ >> code. >> >> If you check how and why the original TI-originated version of the code >> does not follow the linux coding standards, the difficulties we would >> have making a warning-free patch of it should be apparent. > > Past is past. The idea was not to introduce anymore warning code. I agree but if you want to stick to this, then I don't think we can get a readable macro. And I personally prefer readability over warnings generated by checkpatch.pl script. Cheers, Ameya. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-07-14 at 13:05 +0200, ext Menon, Nishanth wrote: > Phil, > > -----Original Message----- > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > > owner@vger.kernel.org] On Behalf Of Phil Carmody > > Sent: Tuesday, July 14, 2009 6:03 AM > > > > On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote: > > > Thanks for the patch, it only has indentation problems, this is the > > checkpatch output: > > > > > > WARNING: suspect code indent for conditional statements (8, 12) > > > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152: > > > + if (DSP_SUCCEEDED(status)) { \ > > > + if (unlikely((src) == NULL) || \ > > > > > > WARNING: line over 80 characters > > > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154: > > > + unlikely(copy_from_user(dest, src, (elements) * > > sizeof(*(dest))))) { \ > > > > > > WARNING: suspect code indent for conditional statements (8, 12) > > > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164: > > > + if (DSP_SUCCEEDED(status)) { \ > > > + if (unlikely((dest) == NULL) || > > \ > > > > > > WARNING: line over 80 characters > > > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166: > > > + unlikely(copy_to_user(dest, src, (elements) * > > sizeof(*(src))))) { \ > > > > > > total: 0 errors, 4 warnings, 48 lines checked > > > > > > Could you please fix that warning please? > > > > I suspect the only way to fix those warnings would either introduce > > other warnings, or \ > Gentle query: Have we already tried this or is it just a suspicion? Being an Englishman, I use various forms of irony more than others do; that particular form's called "litotes". It indeed is not a suspicion - I tried about 4 or 5 variations, none of which were both readable and warning-free. In the end I decided that whatever was closest to the original would be safest. > > would \ > > lead \ > > to \ > > utterly \ > > unread- \ > > able \ > > code. > > > > If you check how and why the original TI-originated version of the code > > does not follow the linux coding standards, the difficulties we would > > have making a warning-free patch of it should be apparent. > > Past is past. The idea was not to introduce anymore warning code. The TI code would trigger the following 4 warnings: WARNING: suspect code indent for conditional statements (4, 12) WARNING: line over 80 characters WARNING: suspect code indent for conditional statements (4, 12) WARNING: line over 80 characters 4 warnings isn't more than 4 warnings, it's no change - and as I mention above, in the absence of an easy fix, that was deliberate. Phil -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c index 86812c6..aaf3019 100644 --- a/drivers/dsp/bridge/pmgr/wcd.c +++ b/drivers/dsp/bridge/pmgr/wcd.c @@ -147,25 +147,29 @@ /* Following two macros should ideally have do{}while(0) */ -#define cp_fm_usr(dest, src, status, elements) \ - if (DSP_SUCCEEDED(status)) {\ - if (unlikely(src == NULL) || \ - unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \ - GT_1trace(WCD_debugMask, GT_7CLASS, \ - "copy_from_user failed, src=0x%x\n", src); \ - status = DSP_EPOINTER ; \ - } \ - } - -#define cp_to_usr(dest, src, status, elements) \ - if (DSP_SUCCEEDED(status)) {\ - if (unlikely(dest == NULL) || \ - unlikely(copy_to_user(dest, src, elements * sizeof(*(src))))) { \ - GT_1trace(WCD_debugMask, GT_7CLASS, \ - "copy_to_user failed, dest=0x%x\n", dest); \ - status = DSP_EPOINTER ;\ - } \ - } +#define cp_fm_usr(dest, src, status, elements) \ + do { \ + if (DSP_SUCCEEDED(status)) { \ + if (unlikely((src) == NULL) || \ + unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \ + GT_1trace(WCD_debugMask, GT_7CLASS, \ + "copy_from_user failed, src=0x%x\n", src); \ + (status) = DSP_EPOINTER ; \ + } \ + } \ + } while (0) + +#define cp_to_usr(dest, src, status, elements) \ + do { \ + if (DSP_SUCCEEDED(status)) { \ + if (unlikely((dest) == NULL) || \ + unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \ + GT_1trace(WCD_debugMask, GT_7CLASS, \ + "copy_to_user failed, dest=0x%x\n", dest); \ + (status) = DSP_EPOINTER ; \ + } \ + } \ + } while (0) /* Device IOCtl function pointer */ struct WCD_Cmd {