| Message ID | 1220847899-22189-1-git-send-email-gerickson@nuovations.com |
|---|---|
| State | Accepted |
| Commit | 85eac85a5bc3bdabf31f148c763ef720292d1e32 |
| Headers | show |
On Sun, 2008-09-07 at 21:24 -0700, Grant Erickson wrote: > Static-qualified all globals except 'main' because they have no use > beyond file scope. > Constant-qualified MTD device and input positional parameter globals. > Constant-qualified argv array. Ack for whole series.
On Mon, Sep 08, 2008 at 08:28:02AM +0300, Artem Bityutskiy wrote: >On Sun, 2008-09-07 at 21:24 -0700, Grant Erickson wrote: >> Static-qualified all globals except 'main' because they have no use >> beyond file scope. >> Constant-qualified MTD device and input positional parameter globals. >> Constant-qualified argv array. > >Ack for whole series. For my benefit, it would be nice if you put the full Acked-by: line in future emails. Then I don't have to go lookup what you typically use for email address, etc in those cases :). I'm going to assmue this is fine: Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com> Yell if it's not. josh
On Mon, 2008-09-08 at 10:17 -0400, Josh Boyer wrote: > For my benefit, it would be nice if you put the full Acked-by: line in > future emails. Then I don't have to go lookup what you typically use > for email address, etc in those cases :). OK, sorry for inconvenience. > Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com> Sure it is, thanks.
Grant Erickson <gerickson@nuovations.com> writes: > +static const char *mtd_device, *img; would it be possible to split this into two declarations? I really don't know if 'img' points to a constant or non-constant char... > -int main(int argc, char **argv) > +int main(int argc, char * const argv[]) Is this really correct? C standard mentions only int main(void) { /* ... */ } int main(int argc, char *argv[]) { /* ... */ } as program entry points. I suggest to cast 'argv' to a corresponding data type when it is used. Enrico
On 9/9/08 6:23 AM, Enrico Scholz wrote: > Grant Erickson <gerickson@nuovations.com> writes: >> +static const char *mtd_device, *img; > > would it be possible to split this into two declarations? I > really don't know if 'img' points to a constant or non-constant > char... As currently implemented, both mtd_device and img point to effectively read-only information. That is, there is no need to modify or to attempt to modify what they point to. Should that change in the future, these can be declared differently; however, for the present, the const qualifier accurately represents program intent. >> -int main(int argc, char **argv) >> +int main(int argc, char * const argv[]) > > Is this really correct? C standard mentions only > > int main(void) { /* ... */ } > int main(int argc, char *argv[]) { /* ... */ } > > as program entry points. I suggest to cast 'argv' to a corresponding > data type when it is used. Those are certainly the two most common prototypes for main. However, because main has no prototype, adding the const qualifier simply conveys current program intent. That is, the strings pointed to by argv are not modified. Regards, Grant
Grant Erickson <gerickson@nuovations.com> writes: >>> +static const char *mtd_device, *img; >> >> would it be possible to split this into two declarations? I >> really don't know if 'img' points to a constant or non-constant >> char... > > As currently implemented, both mtd_device and img point to > effectively read-only information. That is, there is no need to > modify or to attempt to modify what they point to. Sorry, I might be unclear here. My comment was about readability not about correctness; e.g. without studying C standard, it is not obvious to me, whether | char const *a, *b; means | char const *a; | char const *b; or | char const *a; | char *b; When you think that answer to this question is trivial, then please explain 'char * const *a, *b;' ;) >>> -int main(int argc, char **argv) >>> +int main(int argc, char * const argv[]) >> >> Is this really correct? C standard mentions only >> >> int main(void) { /* ... */ } >> int main(int argc, char *argv[]) { /* ... */ } >> >> as program entry points. I suggest to cast 'argv' to a corresponding >> data type when it is used. > > Those are certainly the two most common prototypes for main. However, > because main has no prototype Compiler knows about the two possible prototypes. Perhaps next gcc version or another compiler (icc) complains about the non-standard main(). > adding the const qualifier simply conveys current program > intent. That is, the strings pointed to by argv are not > modified. Then, you can write | int main(int argc, char const * const argv[]) ;) fwiw, the const'ness if argv[] content is violated when calling GNU getopt(3) as it reorders arguments. E.g. | argv[] = { "foo", "-a", "bar", "-c", NULL } is changed by this function to | argv[] = { "foo", "-a", "-c", "bar", NULL } Enrico
On 9/9/08 10:14 AM, Enrico Scholz wrote: > Grant Erickson <gerickson@nuovations.com> writes: > >>> +static const char *mtd_device, *img; >>> >>> would it be possible to split this into two declarations? I >>> really don't know if 'img' points to a constant or non-constant >>> char... >> >> As currently implemented, both mtd_device and img point to >> effectively read-only information. That is, there is no need to >> modify or to attempt to modify what they point to. > > Sorry, I might be unclear here. My comment was about readability > not about correctness; e.g. without studying C standard, it is > not obvious to me, whether > > [ Examples Omitted ] > > > When you think that answer to this question is trivial, then > please explain 'char * const *a, *b;' ;) Fair enough. Patch submitted. >>>> -int main(int argc, char **argv) >>>> +int main(int argc, char * const argv[]) >>> >>> Is this really correct? C standard mentions only >>> >>> int main(void) { /* ... */ } >>> int main(int argc, char *argv[]) { /* ... */ } >>> >>> as program entry points. I suggest to cast 'argv' to a corresponding >>> data type when it is used. > >> adding the const qualifier simply conveys current program >> intent. That is, the strings pointed to by argv are not >> modified. > > Then, you can write > > | int main(int argc, char const * const argv[]) That's certainly possible as well; however, that was not the intent and, as you cite with getopt below, would not accurately reflect how the program respect argv. > fwiw, the const'ness if argv[] content is violated when calling > GNU getopt(3) as it reorders arguments. E.g. > > | argv[] = { "foo", "-a", "bar", "-c", NULL } > > is changed by this function to > > | argv[] = { "foo", "-a", "-c", "bar", NULL } Understood on the operation of getopt. The declared intent was not that order of the cells is constant but that the contents of the cells (i.e. the strings) are. Regards, Grant
diff --git a/nandwrite.c b/nandwrite.c index e745fba..8e0609d 100644 --- a/nandwrite.c +++ b/nandwrite.c @@ -45,32 +45,32 @@ /* * Buffer array used for writing data */ -unsigned char writebuf[MAX_PAGE_SIZE]; -unsigned char oobbuf[MAX_OOB_SIZE]; -unsigned char oobreadbuf[MAX_OOB_SIZE]; +static unsigned char writebuf[MAX_PAGE_SIZE]; +static unsigned char oobbuf[MAX_OOB_SIZE]; +static unsigned char oobreadbuf[MAX_OOB_SIZE]; // oob layouts to pass into the kernel as default -struct nand_oobinfo none_oobinfo = { +static struct nand_oobinfo none_oobinfo = { .useecc = MTD_NANDECC_OFF, }; -struct nand_oobinfo jffs2_oobinfo = { +static struct nand_oobinfo jffs2_oobinfo = { .useecc = MTD_NANDECC_PLACE, .eccbytes = 6, .eccpos = { 0, 1, 2, 3, 6, 7 } }; -struct nand_oobinfo yaffs_oobinfo = { +static struct nand_oobinfo yaffs_oobinfo = { .useecc = MTD_NANDECC_PLACE, .eccbytes = 6, .eccpos = { 8, 9, 10, 13, 14, 15} }; -struct nand_oobinfo autoplace_oobinfo = { +static struct nand_oobinfo autoplace_oobinfo = { .useecc = MTD_NANDECC_AUTOPLACE }; -void display_help (void) +static void display_help (void) { printf("Usage: nandwrite [OPTION] MTD_DEVICE INPUTFILE\n" "Writes to the specified MTD device.\n" @@ -91,7 +91,7 @@ void display_help (void) exit(0); } -void display_version (void) +static void display_version (void) { printf(PROGRAM " " VERSION "\n" "\n" @@ -106,20 +106,20 @@ void display_version (void) exit(0); } -char *mtd_device, *img; -int mtdoffset = 0; -int quiet = 0; -int writeoob = 0; -int markbad = 0; -int autoplace = 0; -int forcejffs2 = 0; -int forceyaffs = 0; -int forcelegacy = 0; -int noecc = 0; -int pad = 0; -int blockalign = 1; /*default to using 16K block size */ - -void process_options (int argc, char *argv[]) +static const char *mtd_device, *img; +static int mtdoffset = 0; +static int quiet = 0; +static int writeoob = 0; +static int markbad = 0; +static int autoplace = 0; +static int forcejffs2 = 0; +static int forceyaffs = 0; +static int forcelegacy = 0; +static int noecc = 0; +static int pad = 0; +static int blockalign = 1; /*default to using 16K block size */ + +static void process_options (int argc, char * const argv[]) { int error = 0; @@ -209,7 +209,7 @@ void process_options (int argc, char *argv[]) /* * Main program */ -int main(int argc, char **argv) +int main(int argc, char * const argv[]) { int cnt, fd, ifd, imglen = 0, pagelen, baderaseblock, blockstart = -1; struct mtd_info_user meminfo;
Static-qualified all globals except 'main' because they have no use beyond file scope. Constant-qualified MTD device and input positional parameter globals. Constant-qualified argv array. Signed-off-by: Grant Erickson <gerickson@nuovations.com>