You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucy.apache.org by Peter Karman <pe...@peknet.com> on 2009/12/09 03:33:45 UTC
Re: [Lucy] Re: quiet gcc warnings
Marvin Humphrey wrote on 11/29/09 6:36 PM:
> On Sat, Nov 28, 2009 at 08:58:04PM -0600, Peter Karman wrote:
>
>>> There's already a PTR2I64 macro in Charmonizer/Probe/Integers, but we probably
>>> need a larger menu:
>>>
>>> PTR2I64
>>> PTR2U64
>>> PTR2I32
>>> PTR2U32
>>> PTR2SIZET
>> sounds reasonable.
>>
>> Should pointers always be converted to unsigned types?
>
> Sometimes if you're subtracting two pointers from each other, you need the
> difference to be a signed result. The type "ptrdiff_t" exists for this
> reason, but it's tricky to use correctly because you have to think about how
> it gets upconverted on different systems.
>
> http://blogs.msdn.com/david_leblanc/archive/2008/09/02/ptrdiff-t-is-evil.aspx
>
that's a good example.
> My approach is to avoid ptrdiff_t altogether and always upconvert all operands
> to i64_t before performing the arithmetic. For example[1]:
>
> static INLINE i64_t
> SI_tell(InStream *self)
> {
> FileWindow *const window = self->window;
> i64_t pos_in_buf = PTR2I64(self->buf) - PTR2I64(window->buf);
> return pos_in_buf + window->offset - self->offset;
> }
>
yes, makes sense.
> A thought: should we only define PTR2I32 and PTR2U32 on 32-bit systems?
> They'd silently discard data on 64-bit systems. That would be bad.
>
> We should force the programmer to either wrap the PTR2x32 conversion in an
> #ifdef, or go through PTR2x64 first and then truncate with an explicit cast
> which verifies their intent.
>
> /* Assume that "start" and "end" cannot be more than I32_MAX apart. */
> #if (SIZEOF_PTR == 4)
> i32_t length = PTR2I32(end) - PTR2I32(start);
> #else
> i64_t a = PTR2I64(start);
> i64_t b = PTR2I64(end);
> i32_t length = (i32_t)(b - a);
> #endif
>
yes, that seems sane.
Shall I have a go at this for my next task, or do you have something else in mind?
--
Peter Karman . http://peknet.com/ . peter@peknet.com
Re: [Lucy] Re: quiet gcc warnings
Posted by Peter Karman <pe...@peknet.com>.
Marvin Humphrey wrote on 12/10/09 7:30 PM:
> Those will allow us to eliminate all the chy int types except for bool_t,
> which we'll replace with "bool". For "bool", we could use a three-part probe:
>
> If the C99 header stdbool.h is available, we pound-include it in
> charmony.h.
>
> Else if the macro "__cplusplus" is defined, then we just use C++'s
> built-in bool type.
>
> Else we typedef "bool" to int and conditionally define "true" and "false",
> just like we're doing now for chy_bool_t.
>
> I'm not sure about that third branch, though, because it could potentially
> conflict with somebody else typedef-ing bool to "char". "chy_bool_t" was
> safe to muck with, but "bool" is not.
>
> So, probably the best move is to just omit the third branch, making either
> stdbool.h or C++ a prerequisite for Lucy.
>
I will tackle this in a separate patch. I agree about avoiding the 3rd branch.
In libswish3 I define a 'boolean' (spelled out long to avoid conflict) as a
char. I expect others out there have done similar.
--
Peter Karman . http://peknet.com/ . peter@peknet.com
Re: [Lucy] Re: quiet gcc warnings
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Thu, Dec 10, 2009 at 09:22:00AM -0800, Marvin Humphrey wrote:
> Great, I'll go work on the following couple of things today:
>
> * Finish phase 1 of the switchover from Boilerplater to Clownfish, so that
> we can eliminate trunk/boilerplater and use only trunk/clownfish.
> * Mod Clownfish to accept the new integer types.
Done.
I didn't include everything from stdint.h (e.g. no int_fast32_t, no
int_least32_t, no intmax_t), because it's not clear that Clownfish ought to
support all that. Just the following for now:
int8_t
int16_t
int32_t
int64_t
uint8_t
uint16_t
uint32_t
uint64_t
Those will allow us to eliminate all the chy int types except for bool_t,
which we'll replace with "bool". For "bool", we could use a three-part probe:
If the C99 header stdbool.h is available, we pound-include it in
charmony.h.
Else if the macro "__cplusplus" is defined, then we just use C++'s
built-in bool type.
Else we typedef "bool" to int and conditionally define "true" and "false",
just like we're doing now for chy_bool_t.
I'm not sure about that third branch, though, because it could potentially
conflict with somebody else typedef-ing bool to "char". "chy_bool_t" was
safe to muck with, but "bool" is not.
So, probably the best move is to just omit the third branch, making either
stdbool.h or C++ a prerequisite for Lucy.
Marvin Humphrey
Re: [Lucy] Re: quiet gcc warnings
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Thu, Dec 10, 2009 at 09:04:35AM -0600, Peter Karman wrote:
> got it. I'll take this on, including opening the JIRA ticket.
Great, I'll go work on the following couple of things today:
* Finish phase 1 of the switchover from Boilerplater to Clownfish, so that
we can eliminate trunk/boilerplater and use only trunk/clownfish.
* Mod Clownfish to accept the new integer types.
Once the two of us finish our respective tasks, it will be OK to start using
the new integer types in the Lucy core.
Marvin Humphrey
Re: [Lucy] Re: quiet gcc warnings
Posted by Peter Karman <pe...@peknet.com>.
Marvin Humphrey wrote on 12/11/09 9:16 PM:
> On Fri, Dec 11, 2009 at 08:59:32PM -0600, Peter Karman wrote:
>> well, I've opened the ticket and started poking around but I am not sure
>> where to include stdint.h and/or test for its existence.
>>
>> Is Charmonizer/Probe/Integers.c where I want to make this addition of
>> include stdint.h?
>
> Yes. Use the HeadCheck_check_header() function to determine whether stdint.h
> is available.
>
> If it's there use ModHand_append_conf() to add the pound-include.
>
> If it's not there, use ModHand_append_conf() to add the typedefs.
>
perfect. patch uploaded to jira.
--
Peter Karman . http://peknet.com/ . peter@peknet.com
Re: [Lucy] Re: quiet gcc warnings
Posted by Peter Karman <pe...@peknet.com>.
Marvin Humphrey wrote on 12/11/09 9:19 PM:
> PS: Oh, and if you can muster the energy, please keep a brainlog as you try to
> figure this out. It would be a valuable refactoring aid.
* I want to basically add this logic to the generated charmony.h file:
#ifdef _HAVE_STDINT_
#include <stdint.h>
#else
typedef .. // add our basic int types here based on Probe/Integer.c
#endif
where do I make this change in Charmonizer so that the code is generated correctly?
* I have 2 file names to go on from the email thread (upon which I must rely
because I can't remember anything anymore. early onset senility. Now, what was I
doing? Oh yes.) Charmonizer/Probe/Integer.c and charmony.h
% find . -name 'charmony.h'
one hit, in perl/charmony.h -- but that file clearly says it is generated by
Charmonizer, so Charmonizer/Probe/Integer.c is my likely bet.
* open Integer.c and Integer.h to poke around. My first instinct is to add the
#include <stdint.h> at the top to go with the other standard includes there, but
since part of the task assumes that file is not always available, I'll need to
avoid my first instinct.
* ah, here is some text I recognize from charmony.h in Integers_run(). This must
be the generator. Write to lucy list to confirm before I dive in.
[… time passes …]
Marvin replies with perfectly clear advice on the functions to use. Exactly what
I need. Also, started this brainlog.
* Knowing which functions to use makes this very easy. The hardest part becomes
where in Integers_run() is most appropriate place to put the new code. I settle
on a spot right below the comment about writing affirmations and typedefs, with
the assumption that Some Day in the Future, the stdint defs will replace all the
chy_* stuff.
* cd perl && make clean && make test
* /me drinks the Charmonizer kool-aid. I get it now. Very nice. It's sort of
like the next evolution of autoconf, with the added love of letting you generate
C via Clownfish.
--
Peter Karman . http://peknet.com/ . peter@peknet.com
Re: [Lucy] Re: quiet gcc warnings
Posted by Marvin Humphrey <ma...@rectangular.com>.
> Yes. Use the HeadCheck_check_header() function to determine whether stdint.h
> is available.
>
> If it's there use ModHand_append_conf() to add the pound-include.
>
> If it's not there, use ModHand_append_conf() to add the typedefs.
PS: Oh, and if you can muster the energy, please keep a brainlog as you try to
figure this out. It would be a valuable refactoring aid.
Marvin Humphrey
Re: [Lucy] Re: quiet gcc warnings
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Fri, Dec 11, 2009 at 08:59:32PM -0600, Peter Karman wrote:
> well, I've opened the ticket and started poking around but I am not sure
> where to include stdint.h and/or test for its existence.
>
> Is Charmonizer/Probe/Integers.c where I want to make this addition of
> include stdint.h?
Yes. Use the HeadCheck_check_header() function to determine whether stdint.h
is available.
If it's there use ModHand_append_conf() to add the pound-include.
If it's not there, use ModHand_append_conf() to add the typedefs.
Marvin Humphrey
Re: [Lucy] Re: quiet gcc warnings
Posted by Peter Karman <pe...@peknet.com>.
Peter Karman wrote on 12/10/09 9:04 AM:
> Marvin Humphrey wrote on 12/9/09 6:15 PM:
>> On Tue, Dec 08, 2009 at 09:24:44PM -0800, Marvin Humphrey wrote:
>>
>>> So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
>>>
>>> We could actually generate a complete and usable stdint.h on systems
>>> where it
>>> isn't available using no more compilation tests than we already have
>>> going in
>>> Charmonizer/Probe/Integers.c. But as a quick and dirty fix, we can
>>> either
>>> pound-include stdint.h in charmony.h if it's there, or change the
>>> naming of
>>> our typedefs if it's not.
>>
>> PS...
>>
>> If you take this on, just add rather than replace the typedefs and
>> call it a
>> day. If we try to remove the old defs right now, we'll get massive
>> failure
>> throughout the rest of the system, and it's not worth trying to fix
>> that all
>> at once.
>>
>> After the stdint.h typedefs are in Charmonizer, we can expand
>> Clownfish to
>> accept the new type. Then we replace all instances in core and the Perl
>> bindings. Lastly, once the substitution has completed, we can safely
>> remove
>> support for the chy int types from Clownfish and Charmonizer.
>>
>
> got it. I'll take this on, including opening the JIRA ticket.
>
>
well, I've opened the ticket and started poking around but I am not sure where
to include stdint.h and/or test for its existence.
Is Charmonizer/Probe/Integers.c where I want to make this addition of include
stdint.h?
It *seems* like I would add something in Integers_run() in Integers.c. Is that
right?
--
Peter Karman . http://peknet.com/ . peter@peknet.com
Re: [Lucy] Re: quiet gcc warnings
Posted by Peter Karman <pe...@peknet.com>.
Marvin Humphrey wrote on 12/9/09 6:15 PM:
> On Tue, Dec 08, 2009 at 09:24:44PM -0800, Marvin Humphrey wrote:
>
>> So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
>>
>> We could actually generate a complete and usable stdint.h on systems where it
>> isn't available using no more compilation tests than we already have going in
>> Charmonizer/Probe/Integers.c. But as a quick and dirty fix, we can either
>> pound-include stdint.h in charmony.h if it's there, or change the naming of
>> our typedefs if it's not.
>
> PS...
>
> If you take this on, just add rather than replace the typedefs and call it a
> day. If we try to remove the old defs right now, we'll get massive failure
> throughout the rest of the system, and it's not worth trying to fix that all
> at once.
>
> After the stdint.h typedefs are in Charmonizer, we can expand Clownfish to
> accept the new type. Then we replace all instances in core and the Perl
> bindings. Lastly, once the substitution has completed, we can safely remove
> support for the chy int types from Clownfish and Charmonizer.
>
got it. I'll take this on, including opening the JIRA ticket.
--
Peter Karman . http://peknet.com/ . peter@peknet.com
Re: [Lucy] Re: quiet gcc warnings
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Tue, Dec 08, 2009 at 09:24:44PM -0800, Marvin Humphrey wrote:
> So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
>
> We could actually generate a complete and usable stdint.h on systems where it
> isn't available using no more compilation tests than we already have going in
> Charmonizer/Probe/Integers.c. But as a quick and dirty fix, we can either
> pound-include stdint.h in charmony.h if it's there, or change the naming of
> our typedefs if it's not.
PS...
If you take this on, just add rather than replace the typedefs and call it a
day. If we try to remove the old defs right now, we'll get massive failure
throughout the rest of the system, and it's not worth trying to fix that all
at once.
After the stdint.h typedefs are in Charmonizer, we can expand Clownfish to
accept the new type. Then we replace all instances in core and the Perl
bindings. Lastly, once the substitution has completed, we can safely remove
support for the chy int types from Clownfish and Charmonizer.
Marvin Humphrey
Re: [Lucy] Re: quiet gcc warnings
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Tue, Dec 08, 2009 at 08:33:45PM -0600, Peter Karman wrote:
> >My approach is to avoid ptrdiff_t altogether and always upconvert all
> >operands
> >to i64_t before performing the arithmetic. For example[1]:
> >
> > static INLINE i64_t
> > SI_tell(InStream *self)
> > {
> > FileWindow *const window = self->window;
> > i64_t pos_in_buf = PTR2I64(self->buf) - PTR2I64(window->buf);
> > return pos_in_buf + window->offset - self->offset;
> > }
> >
>
> yes, makes sense.
I should mention for the record that technically it's a violation of the C
standard to perform arbitrary pointer math. This is officially undefined:
char *foo = (char*)malloc(2);
foo = foo--;
The only officially valid addresses with regards to that pointer are:
foo
foo + 1
foo + 2
See:
http://stackoverflow.com/questions/784764/what-c-compilers-have-pointer-subtraction-underflows
However, on common modern systems, you get away with it... and in fact
InStream's design depends on that construct working.
> >A thought: should we only define PTR2I32 and PTR2U32 on 32-bit systems?
> >They'd silently discard data on 64-bit systems. That would be bad.
> >
> >We should force the programmer to either wrap the PTR2x32 conversion in an
> >#ifdef, or go through PTR2x64 first and then truncate with an explicit cast
> >which verifies their intent.
> >
> > /* Assume that "start" and "end" cannot be more than I32_MAX apart. */
> > #if (SIZEOF_PTR == 4)
> > i32_t length = PTR2I32(end) - PTR2I32(start);
> > #else
> > i64_t a = PTR2I64(start);
> > i64_t b = PTR2I64(end);
> > i32_t length = (i32_t)(b - a);
> > #endif
> >
>
> yes, that seems sane.
>
> Shall I have a go at this for my next task, or do you have something else
> in mind?
That would be a good one.
Anything that moves us towards the goal of an easy to understand and use
Charmonizer code base would be handy.
The next step is probably for me to redraft the top level docs -- Charmonizer
has the functionality it needs, but the organization can be improved. Then if
I can persuade you and Nate to critique them, we'll refactor accordingly.
Since Charmonizer is an internal API, it doesn't have to be perfect, but it's
a dialect of C that Lucy contributors will have to familiarize themselves with
and the less effort that takes the better.
Hmmm....
Along those lines, it would be nice if we could phase out all the
Charmonizer-specific integer types and replace them with types from the C99
header stdint.h.
http://www.opengroup.org/onlinepubs/000095399/basedefs/stdint.h.html
So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
We could actually generate a complete and usable stdint.h on systems where it
isn't available using no more compilation tests than we already have going in
Charmonizer/Probe/Integers.c. But as a quick and dirty fix, we can either
pound-include stdint.h in charmony.h if it's there, or change the naming of
our typedefs if it's not.
Marvin Humphrey