| Message ID | 1395313907-25318-2-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3f9d720a4d29834b8362d820537acb042c17e33c |
| Headers | show |
On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote: > Reported-by: Brian Norris <computersforpeace@gmail.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> Why are these useless? Even if the object code produced is the same, these serve as information to the human reader about how the arguments are used. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, 20 Mar 2014, Joe Perches wrote: > On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote: > > Reported-by: Brian Norris <computersforpeace@gmail.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > Why are these useless? > > Even if the object code produced is the same, > these serve as information to the human reader > about how the arguments are used. https://lkml.org/lkml/2014/3/20/69
On Thu, 2014-03-20 at 11:41 +0000, Lee Jones wrote: > On Thu, 20 Mar 2014, Joe Perches wrote: > > > On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote: > > > Reported-by: Brian Norris <computersforpeace@gmail.com> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > Why are these useless? > > > > Even if the object code produced is the same, > > these serve as information to the human reader > > about how the arguments are used. > > https://lkml.org/lkml/2014/3/20/69 > From that email: suggestion: const uint8_t *const buf => const uint8_t *buf const uint32_t size => uint32_t size const uint32_t offset => uint32_t offset What was done: -static int stfsm_write_fifo(struct stfsm *fsm, - const uint32_t *buf, const uint32_t size) +static int stfsm_write_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size) Note the removal of the const from uint32_t *buf Why? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> Good catch. That wasn't my intention. +1 > > Why? An oversight. > I can let Lee speak for himself. But I'm thinking I'll squash this diff > in, if no one minds: > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Patch looks good to me: Acked-by: Lee Jones <lee.jones@linaro.org>
On Thu, Mar 20, 2014 at 5:03 AM, Lee Jones <lee.jones@linaro.org> wrote: >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > Patch looks good to me: > Acked-by: Lee Jones <lee.jones@linaro.org> Squashed into $SUBJECT patch and pushed to l2-mtd.git. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, 2014-03-20 at 12:03 +0000, Lee Jones wrote: > > Good catch. That wasn't my intention. > > > Why? > An oversight. That's still not an explanation. Why, unless cast away by the code itself, is const removal a good thing? It does serve as an indication to a reader what the code does with the argument. About the only reason I can think of arguing in favor of removal is inconsistent application of const within the module. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Mar 20, 2014 at 5:13 AM, Joe Perches <joe@perches.com> wrote: > On Thu, 2014-03-20 at 12:03 +0000, Lee Jones wrote: >> > Good catch. That wasn't my intention. >> > > Why? >> An oversight. > > That's still not an explanation. > > Why, unless cast away by the code itself, is > const removal a good thing? It's not so much removal as it is review of the initial driver merge. I'd contend that const was applied somewhat thoughtlessly originally, and it didn't really serve a good purpose. > It does serve as an indication to a reader what > the code does with the argument. > > About the only reason I can think of arguing in > favor of removal is inconsistent application of > const within the module. That's one good reason. And not only consistency within the modules, but consistency within the subsystem (and the kernel at large, really). There's rarely a case of a const function parameter. And I'm sure there are numerous function parameters which could potentially be marked 'const'. I also don't think that a function parameter is the right place to mark const like this. Function arguments are always pass-by-value, so this 'const' tells users (callers) nothing useful. It only provides useless constraints on what the function can do with its copy of the parameter. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, 2014-03-20 at 05:41 -0700, Brian Norris wrote: > I also don't think that a function parameter is the right place to > mark const like this. Function arguments are always pass-by-value, so > this 'const' tells users (callers) nothing useful. It only provides > useless constraints on what the function can do with its copy of the > parameter. Again, that's not useless information. And as you've seen, just making these changes can be error prone. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> > > Good catch. That wasn't my intention. > > > > Why? > > An oversight. > > That's still not an explanation. > > Why, unless cast away by the code itself, is > const removal a good thing? > > It does serve as an indication to a reader what > the code does with the argument. > > About the only reason I can think of arguing in > favor of removal is inconsistent application of > const within the module. Your qualm is not with me in this instance. I don't ever mind holding my hands up to mistakes I make or standing up for decisions I believe in, but in this case I'm merely satisfying the wishes of a maintainer I'm submitting code to. I am strongly un-opinionated about these particular changes.
On Thu, Mar 20, 2014 at 05:44:45AM -0700, Joe Perches wrote: > On Thu, 2014-03-20 at 05:41 -0700, Brian Norris wrote: > > I also don't think that a function parameter is the right place to > > mark const like this. Function arguments are always pass-by-value, so > > this 'const' tells users (callers) nothing useful. It only provides > > useless constraints on what the function can do with its copy of the > > parameter. > > Again, that's not useless information. For local, pass-by-value function arguments (i.e., constant data, or constant pointers to data), I respectfully disagree. (For "pointers to constant data", I completely agree that the 'const' info is useful.) > And as you've seen, just making these changes > can be error prone. Thank you for catching our mistake now. But I don't think that is relevant; just because you caught an error doesn't mean that the change (primarily for consistency's sake) should be avoided entirely. Unless you have a more convincing argument, this code will remain as-is. Regards, Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c index aefd48d..dcc1dc3 100644 --- a/drivers/mtd/devices/st_spi_fsm.c +++ b/drivers/mtd/devices/st_spi_fsm.c @@ -785,8 +785,7 @@ static void stfsm_wait_seq(struct stfsm *fsm) dev_err(fsm->dev, "timeout on sequence completion\n"); } -static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf, - const uint32_t size) +static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size) { uint32_t remaining = size >> 2; uint32_t avail; @@ -811,8 +810,7 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf, } } -static int stfsm_write_fifo(struct stfsm *fsm, - const uint32_t *buf, const uint32_t size) +static int stfsm_write_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size) { uint32_t words = size >> 2; @@ -1544,8 +1542,8 @@ static int stfsm_read(struct stfsm *fsm, uint8_t *buf, uint32_t size, return 0; } -static int stfsm_write(struct stfsm *fsm, const uint8_t *const buf, - const uint32_t size, const uint32_t offset) +static int stfsm_write(struct stfsm *fsm, const uint8_t *buf, + uint32_t size, uint32_t offset) { struct stfsm_seq *seq = &fsm->stfsm_seq_write; uint32_t data_pads; @@ -1670,7 +1668,7 @@ static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len, return 0; } -static int stfsm_erase_sector(struct stfsm *fsm, const uint32_t offset) +static int stfsm_erase_sector(struct stfsm *fsm, uint32_t offset) { struct stfsm_seq *seq = &stfsm_seq_erase_sector; int ret;
Reported-by: Brian Norris <computersforpeace@gmail.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mtd/devices/st_spi_fsm.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)