[sqlite] Magic number in sqlite source code
Hello, Richard! Probably PTR_SZ or PAGE_PTR_SZ would be good? I feel confusing about OVFL_. Why size of the pointer to the first overflow page could be different from size of another pointers? Sorry, I'm far from the code and should not discourse. Just have an idea that name should be as common (generic) as possible (to follow Occam's razor). And it's interesting where does it(4) come from? sizeof(int), for file we should fix it, right? (comment to define). >OVFL_PTR_SZ might be a better name. 4 is the size (in bytes) used by >the pointer to the first overflow page that occurs at the end of the >on-page record. Happy New Year! Thanks, Max 2015-12-31 2:54 GMT+03:00 Domingo Alvarez Duarte : > Hello Duncan ! > > I saw a very good point on your suggestion ! > > I'll use it when writing/refactoring code. > > Thanks a lot ! > > > > Wed Dec 30 2015 11:51:54 pm CET CET from "Darren Duncan" > > Subject: Re: [sqlite] Magic number in sqlite > >source code > > > > On 2015-12-30 12:51 PM, Richard Hipp wrote: > > > >>On 12/30/15, Richard Hipp wrote: > >> > >>>I'll continue look for an alternative way to make the intent of the > >>> code clearer. > >>> > > >> See https://www.sqlite.org/src/info/1541607d458069f5 for another > >> attempt at removing magic numbers. But I don't like it It seems to > >> complicate more than it clarifies. My current thinking is that the > >> code should remain as it is on trunk. > >> > > > While kludgy itself, a possible compromise is to still use a named > >constant / > > macro but have '4' in the name of the macro, eg like 'SOME_FOO_4' where > the > > > > SOME_FOO is a semblance of descriptive and the 4 says what the value is > so > >you > > don't have to look it up. The key thing is that there may be multiple > >reasons > > to use the value 4 in a program and the named constant is illustrating > >which > > reason it is. If you change the value of the constant then you would also > > rename this particular constant to match the new value, but the key thing > >is you > > have something easily look-upable that shows all the 4 are connected. -- > >Darren > > Duncan > > > > ___ > > sqlite-users mailing list > > sqlite-users at mailinglists.sqlite.org > > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users > > > > > > > > > > > > ___ > sqlite-users mailing list > sqlite-users at mailinglists.sqlite.org > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users >
[sqlite] Magic number in sqlite source code
Hello Duncan ! I saw a very good point on your suggestion ! I'll use it when writing/refactoring code. Thanks a lot ! ? > Wed Dec 30 2015 11:51:54 pm CET CET from "Darren Duncan" > Subject: Re: [sqlite] Magic number in sqlite >source code > > On 2015-12-30 12:51 PM, Richard Hipp wrote: > >>On 12/30/15, Richard Hipp wrote: >> >>>I'll continue look for an alternative way to make the intent of the >>> code clearer. >>> >> See https://www.sqlite.org/src/info/1541607d458069f5 for another >> attempt at removing magic numbers. But I don't like it It seems to >> complicate more than it clarifies. My current thinking is that the >> code should remain as it is on trunk. >> > While kludgy itself, a possible compromise is to still use a named >constant / > macro but have '4' in the name of the macro, eg like 'SOME_FOO_4' where the > > SOME_FOO is a semblance of descriptive and the 4 says what the value is so >you > don't have to look it up. The key thing is that there may be multiple >reasons > to use the value 4 in a program and the named constant is illustrating >which > reason it is. If you change the value of the constant then you would also > rename this particular constant to match the new value, but the key thing >is you > have something easily look-upable that shows all the 4 are connected. -- >Darren > Duncan > > ___ > sqlite-users mailing list > sqlite-users at mailinglists.sqlite.org > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users > > > ?
[sqlite] Magic number in sqlite source code
Hello Richard !
Sorry again for the mess with format and missing parameter on the maybe
more friendly function "ptrmapPutOvflPtr" !
I decided to really look at the code now (before I looked at the diffs and
saw so many magic numbers "4").
I know that what I'm saying here not always I follow for n reasons but thanks
for giving me a chance to take a moment and reflect on this that do not seem
a problem when writing but man when we come back after 1 or more days it's
not easy to grasp what it mean.
For example this function:
static void ptrmapPutOvflPtr(MemPage *pPage, u8 *pCell, int *pRC){
? CellInfo info;
? if( *pRC ) return;
? assert( pCell!=0 );
? pPage->xParseCell(pPage, pCell, &info);
? if( info.nLocalpBt, ovfl, PTRMAP_OVERFLOW1, pPage->pgno, pRC);
? }
}
I would like to see a less verbose version that hide through "inline
function/macro" parameters that can be derived making the code easier to read
and understand with less mental effort (or with mental effort directed to the
forest instead of the trees where appropriate).
static void ptrmapPutOvflPtr(MemPage *pPage, u8 *pCell, int *pRC){
? CellInfo info;
? if( *pRC ) return;
? assert( pCell!=0 );
? pPage->xParseCell(pPage, pCell, &info);
? if( info.nLocal
[sqlite] Magic number in sqlite source code
Hello Richard !
!!! Last message got messed format, sorry !!!
I decided to really look at the code now (before I looked at the diffs and
saw so many magic numbers "4").
I know that what I'm saying here not always I follow for n reasons but thanks
for giving me a chance to take a moment and reflect on this that do not seem
a problem when writing but man when we come back after 1 or more days it's
not easy to grasp what it mean.
For example this function:
static void ptrmapPutOvflPtr(MemPage *pPage, u8 *pCell, int *pRC){
? CellInfo info;
? if( *pRC ) return;
? assert( pCell!=0 );
? pPage->xParseCell(pPage, pCell, &info);
? if( info.nLocalpBt, ovfl, PTRMAP_OVERFLOW1, pPage->pgno, pRC);
? }
}
I would like to see a less verbose version that hide through "inline
function/macro" parameters that can be derived making the code easier to read
and understand with less mental effort (or with mental effort directed to the
forest instead of the trees where appropriate).
static void ptrmapPutOvflPtr(MemPage *pPage, u8 *pCell, int *pRC){
? CellInfo info;
? if( *pRC ) return;
? assert( pCell!=0 );
? pPage->xParseCell(pPage, pCell, &info);
? if( info.nLocal
[sqlite] Magic number in sqlite source code
Hello Richard !
I decided to really look at the code now (before I looked at the diffs and
saw so many magic numbers "4").
I know that what I'm saying here not always I follow for n reasons but thanks
for giving me a chance to take a moment and reflect on this that do not seem
a problem when writing but man when we come back after 1 or more days it's
not easy to grasp what it mean.
For example this function: ? static void ptrmapPutOvflPtr(MemPage *pPage,
u8 *pCell, int *pRC){CellInfo info;if( *pRC ) return;assert(
pCell!=0 );pPage->xParseCell(pPage, pCell, &info);if(
info.nLocalpBt, ovfl,
PTRMAP_OVERFLOW1, pPage->pgno, pRC);} }
I would like to see a less verbose version that hide through "inline
function/macro" parameters that can be derived making the code easier to read
and understand with less mental effort (or with mental effort directed to the
forest instead of the trees where appropriate). static void
ptrmapPutOvflPtr(MemPage *pPage, u8 *pCell, int *pRC){CellInfo info;
if( *pRC ) return;assert( pCell!=0 );pPage->xParseCell(pPage, pCell,
&info);if( info.nLocal
[sqlite] Magic number in sqlite source code
Hello Richard ! It's hard to come up with names but as it is on https://www.sqlite.org/src/info/1541607d458069f5 I think that's a good improvement to make the code easier to follow and DRY. This approach has at least 2 benefits: 1- Is easy to see what parts of the code depend on PTRMAP_OVERFLOW1 value (by reading the code or searching) 2- If for some reason a change on the "struct CellInfo" is made that need a different value for PTRMAP_OVERFLOW1, only one place need to be changed. ? Also the macro OvflOffset(X) probably could be defined using the PTRMAP_OVERFLOW1. #define PTRMAP_OVERFLOW1 4 #define OvflOffset(X) ((X)->nSize-PTRMAP_OVERFLOW1) Cheers ! > Wed Dec 30 2015 9:51:52 pm CET CET from "Richard Hipp" >Subject: Re: [sqlite] Magic number in sqlite source code > > On 12/30/15, Richard Hipp wrote: > > >>I'll continue look for an alternative way to make the intent of the >> code clearer. >> >> > See https://www.sqlite.org/src/info/1541607d458069f5 for another > attempt at removing magic numbers. But I don't like it. It seems to > complicate more than it clarifies. My current thinking is that the > code should remain as it is on trunk. > -- > D. Richard Hipp > drh at sqlite.org > ___ > sqlite-users mailing list > sqlite-users at mailinglists.sqlite.org > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users > > ?
[sqlite] Magic number in sqlite source code
Hello Richard ! I just saw this commit https://www.sqlite.org/src/info/6a4cfc7ab62046eb and noticed you've been using magic numbers would it be better to use a macro instead ? I think for other people (and maybe yourself) would be easier to see something like "INFO_SIZE_ADJUST" (or any meaningful name) instead of "4". This also happen on other places, although I understand that is hard to maintain consistency (there is several places where sqlite3 do use macros instead of magic numbers). Cheers ! in src/btree.c 1054 pInfo->nSize = (u16)(&pInfo->pPayload[pInfo->nLocal] - pCell) + 4; 1079 assert( pPage->childPtrSize==4 ); 1083 pInfo->nSize = 4 + getVarint(&pCell[4], (u64*)&pInfo->nKey); 1153 if( pInfo->nSize<4 ) pInfo->nSize = 4; 1191 if( pInfo->nSize<4 ) pInfo->nSize = 4; 1309 Pgno ovfl = get4byte(&pCell[info.nSize-4]); 3349 && iFrom==get4byte(pCell+info.nSize-4) 5999 ovflPgno = get4byte(pCell + info.nSize - 4); 6000 assert( pBt->usableSize > 4 ); 6001 ovflPageSize = pBt->usableSize - 4; 6858 Pgno ovfl = get4byte(&z[info.nSize-4]); 9164 assert( pc + info.nSize - 4 <= usableSize ); 9166 pgnoOvfl = get4byte(&pCell[info.nSize - 4]);
[sqlite] Magic number in sqlite source code
On 12/30/15, Richard Hipp wrote: > > I'll continue look for an alternative way to make the intent of the > code clearer. > See https://www.sqlite.org/src/info/1541607d458069f5 for another attempt at removing magic numbers. But I don't like it. It seems to complicate more than it clarifies. My current thinking is that the code should remain as it is on trunk. -- D. Richard Hipp drh at sqlite.org
[sqlite] Magic number in sqlite source code
On 12/30/15, Domingo Alvarez Duarte wrote: > Hello Richard ! > > I just saw this commit https://www.sqlite.org/src/info/6a4cfc7ab62046eb and > noticed you've been using magic numbers would it be better to use a macro > instead ? > > I think for other people (and maybe yourself) would be easier to see > something like "INFO_SIZE_ADJUST" (or any meaningful name) instead of "4". OVFL_PTR_SZ might be a better name. 4 is the size (in bytes) used by the pointer to the first overflow page that occurs at the end of the on-page record. I spent some time editing the code - substituting OVFL_PTR_SZ for 4 in appropriate places. But after looking at that for a while, I felt like OVFL_PTR_SZ obscured more than it clarified. So I typed "fossil revert" to go back to the code as it stands. I'll continue look for an alternative way to make the intent of the code clearer. > > > This also happen on other places, although I understand that is hard to > maintain consistency (there is several places where sqlite3 do use macros > instead of magic numbers). > > Cheers ! > in src/btree.c > > 1054 pInfo->nSize = (u16)(&pInfo->pPayload[pInfo->nLocal] - pCell) + 4; > 1079 assert( pPage->childPtrSize==4 ); > 1083 pInfo->nSize = 4 + getVarint(&pCell[4], (u64*)&pInfo->nKey); > 1153 if( pInfo->nSize<4 ) pInfo->nSize = 4; > 1191 if( pInfo->nSize<4 ) pInfo->nSize = 4; > 1309 Pgno ovfl = get4byte(&pCell[info.nSize-4]); > 3349 && iFrom==get4byte(pCell+info.nSize-4) > 5999 ovflPgno = get4byte(pCell + info.nSize - 4); > 6000 assert( pBt->usableSize > 4 ); > 6001 ovflPageSize = pBt->usableSize - 4; > 6858 Pgno ovfl = get4byte(&z[info.nSize-4]); > 9164 assert( pc + info.nSize - 4 <= usableSize ); > 9166 pgnoOvfl = get4byte(&pCell[info.nSize - 4]); > > ___ > sqlite-users mailing list > sqlite-users at mailinglists.sqlite.org > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users > -- D. Richard Hipp drh at sqlite.org
[sqlite] Magic number in sqlite source code
On 2015-12-30 12:51 PM, Richard Hipp wrote: > On 12/30/15, Richard Hipp wrote: >> I'll continue look for an alternative way to make the intent of the >> code clearer. > > See https://www.sqlite.org/src/info/1541607d458069f5 for another > attempt at removing magic numbers. But I don't like it. It seems to > complicate more than it clarifies. My current thinking is that the > code should remain as it is on trunk. While kludgy itself, a possible compromise is to still use a named constant / macro but have '4' in the name of the macro, eg like 'SOME_FOO_4' where the SOME_FOO is a semblance of descriptive and the 4 says what the value is so you don't have to look it up. The key thing is that there may be multiple reasons to use the value 4 in a program and the named constant is illustrating which reason it is. If you change the value of the constant then you would also rename this particular constant to match the new value, but the key thing is you have something easily look-upable that shows all the 4 are connected. -- Darren Duncan
[sqlite] Magic number in sqlite source code
Hello !
I was looking at this particular commit
https://www.sqlite.org/src/info/0ea6e5c9fc6b1dd1 then I realize the usage of
magic number through sqlite3 source code like the one bellow, it's not good
practice to avoid then ?
Cheers !
= case PragTyp_STATS: { static const char *azCol[] = { "table",
"index", "width", "height" }; Index *pIdx; HashElem *i; v =
sqlite3GetVdbe(pParse);pParse->nMem = 4; ///first appearance of
the a magic number (sizeof(azCol)/sizeof(char*))
sqlite3CodeVerifySchema(pParse, iDb); setAllColumnNames(v, 4, azCol);
second appearance of a magic number described above
for(i=sqliteHashFirst(&pDb->pSchema->tblHash); i; i=sqliteHashNext(i)){
Table *pTab = sqliteHashData(i);sqlite3VdbeAddOp4(v, OP_String8, 0,
1, 0, pTab->zName, 0);sqlite3VdbeAddOp2(v, OP_Null, 0, 2);
sqlite3VdbeAddOp2(v, OP_Integer,
(int)sqlite3LogEstToInt(pTab->szTabRow), 3);sqlite3VdbeAddOp2(v,
OP_Integer,
=
[sqlite] Magic number in sqlite source code
On Sep 3, 2015 4:17 AM, "Domingo Alvarez Duarte"
wrote:
>
> Hello !
>
> I was looking at this particular commit
> https://www.sqlite.org/src/info/0ea6e5c9fc6b1dd1 then I realize the usage
of
> magic number through sqlite3 source code like the one bellow, it's not
good
> practice to avoid then ?
Generally yes, but like most decisions made in programming, there are
exceptions. Much like goto: avoid it when nothing better is available, but
use it when it is the right tool.
Besides, I have seen code that is harder to read when certain magic numbers
are replaced by named constants. It isn't common, but it happens.
>
> Cheers !
>
> = case PragTyp_STATS: { static const char *azCol[] = { "table",
> "index", "width", "height" }; Index *pIdx; HashElem *i; v =
> sqlite3GetVdbe(pParse);pParse->nMem = 4; ///first appearance
of
> the a magic number (sizeof(azCol)/sizeof(char*))
> sqlite3CodeVerifySchema(pParse, iDb); setAllColumnNames(v, 4, azCol);
> second appearance of a magic number described above
> for(i=sqliteHashFirst(&pDb->pSchema->tblHash); i; i=sqliteHashNext(i)){
> Table *pTab = sqliteHashData(i);sqlite3VdbeAddOp4(v, OP_String8,
0,
> 1, 0, pTab->zName, 0);sqlite3VdbeAddOp2(v, OP_Null, 0, 2);
> sqlite3VdbeAddOp2(v, OP_Integer,
> (int)sqlite3LogEstToInt(pTab->szTabRow), 3);sqlite3VdbeAddOp2(v,
> OP_Integer,
>
> =
> ___
> sqlite-users mailing list
> sqlite-users at mailinglists.sqlite.org
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
