You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Lucian Adrian Grijincu <lu...@gmail.com> on 2007/10/14 03:16:56 UTC
some simplifications for the Netware apr_thread_create code
Just removing some unused code.
APR_SUCCESS is deemed to be zero as I understand it, so no use in
checking for it explicitly.
Index: threadproc/netware/thread.c
===================================================================
--- threadproc/netware/thread.c (revision 584412)
+++ threadproc/netware/thread.c (working copy)
@@ -125,19 +125,14 @@
/* int *error */ &stat);
- stat = NXContextSetName(
+ NXContextSetName(
/* NXContext_t ctx */ (*new)->ctx,
/* const char *name */ threadName);
- stat = NXThreadCreate(
+ return NXThreadCreate(
/* NXContext_t context */ (*new)->ctx,
/* long flags */ flags,
/* NXThreadId_t *thread_id */ &(*new)->td);
-
- if(stat==0)
- return APR_SUCCESS;
-
- return(stat);// if error
}
apr_os_thread_t apr_os_thread_current()
Re: some simplifications for the Netware apr_thread_create code
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
William A. Rowe, Jr. wrote:
> Lucian Adrian Grijincu wrote:
>> Just removing some unused code.
>> APR_SUCCESS is deemed to be zero as I understand it, so no use in
>> checking for it explicitly.
>
> Untrue, because it makes things clearer to the reader.
>
> The compiler can't care less if you compare (rv == 0) or (!rv),
> it optimizes it the same way.
It doesn't matter anyways - those aren't apr_ functions, their value of
zero is particular to Netware semantics.
Re: some simplifications for the Netware apr_thread_create code
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/14/07, Lucian Adrian Grijincu <lu...@gmail.com> wrote:
> On 10/14/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> > Lucian Adrian Grijincu wrote:
> > > Just removing some unused code.
> > > APR_SUCCESS is deemed to be zero as I understand it, so no use in
> > > checking for it explicitly.
> >
> > Untrue, because it makes things clearer to the reader.
> >
>
and I find things like:
x = smth();
x = smth_else();
use(x);
more confusing than
smth();
x = smth_else();
use(x);
Even though then compiler might optimize away the first attribution.
> isn't this why they invented comments?
>
> why do:
>
> if(x==0)
> return 0;
> return x;
>
> when you can do:
> return x; //and place a witty comment here?
>
> saves a cmp and a jump.
>
> > The compiler can't care less if you compare (rv == 0) or (!rv),
> > it optimizes it the same way.
> >
>
> What !rv? i don't wanna do the comparison at all.
>
--
Lucian
Re: some simplifications for the Netware apr_thread_create code
Posted by Friedrich Dominicus <fr...@q-software-solutions.de>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> writes:
>>>> why [...] when you can do:
>>>> return x; //and place a witty comment here?
>>>>
>>>> saves a cmp and a jump.
>>> Just forewarn - // isn't portable, please don't litter :) More
>than happy to see
It's in the C standard....
Regards
Friedrich
Re: some simplifications for the Netware apr_thread_create
code
Posted by Brad Nicholes <BN...@novell.com>.
>>> On 10/14/2007 at 5:18 PM, in message
<ac...@mail.gmail.com>, "Lucian Adrian
Grijincu" <lu...@gmail.com> wrote:
> OK, I've attached a patch, but I really feel awkward submitting a
> patch for something:
> a) I haven't compiled
> b) I haven't tested
> c) I have no experience working with it
> d) I couldn't find any useful information about on Google/Koders
> (apparently apr, apache MPM and a "gthr" project are the only known
> open source projects that use some of these functions).
>
> I hope this is the most blindfolded patch you'll ever see from me.
>
> BTW, is there fear of goto amongst APR devs? I've gotten used to the
> Linux kernel coding style and use it where I see fit. I've seen it is
> used in some places inside APR, but if you don't like it I'll clean it
> up and resubmit (I haven't found on
> http://httpd.apache.org/dev/styleguide.html any goto related
> reference).
>
The patch doesn't actually do much to simplify the code. NXContextAlloc() simply allocates the environment for the thread to run. If it fails, the only real error message is that there wasn't enough memory. While this seems significant, it will be picked up later in the call to NXThreadCreate() which will produce the real error that we want to pass back to the caller. NXContextSetName() just gives a name to the thread mainly for debugging purposes. Whether it succeeds or fails has no impact on the thread execution. Finally, NXThreadCreate() is the real guts of the whole thing and the error code that is returned from it, is really what the caller is interested in. Finally, since the thread context was allocated as NX_CTX_NORMAL, the internal thread management will automatically take care of cleaning up the context if NXThreadCreate() fails. So even though this function could be coded slightly more efficiently, in the end there isn't any real gain from doing so.
Brad
Brad
Re: some simplifications for the Netware apr_thread_create code
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/14/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Lucian Adrian Grijincu wrote:
> > On 10/14/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> >> Just forewarn - // isn't portable, please don't litter :) More than happy to see
> >> /* */ style comments.
> >
> > Sorry for being ignorant :). I'll try to remember that.
>
> I wouldn't have said anything if it wasn't the #2 most common mistake, right after
> using tabs instead of spaces :) I know - MS spoils us by calling their C++ compiler
> a C compiler ;-)
>
Well, this file is VERY "tab" littered.
> > I think someone with Netware experience should review this code.
>
> Yes, if you weren't aware, patch is very tolerant of # comments, so you can attach
> your patch with something like...
I wasn't, thank you very much! (Truth be told, APR is the only open
source project I've contributed to and I know I still have much to
learn :)
OK, I've attached a patch, but I really feel awkward submitting a
patch for something:
a) I haven't compiled
b) I haven't tested
c) I have no experience working with it
d) I couldn't find any useful information about on Google/Koders
(apparently apr, apache MPM and a "gthr" project are the only known
open source projects that use some of these functions).
I hope this is the most blindfolded patch you'll ever see from me.
BTW, is there fear of goto amongst APR devs? I've gotten used to the
Linux kernel coding style and use it where I see fit. I've seen it is
used in some places inside APR, but if you don't like it I'll clean it
up and resubmit (I haven't found on
http://httpd.apache.org/dev/styleguide.html any goto related
reference).
--
Lucian
Re: some simplifications for the Netware apr_thread_create code
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
> On 10/14/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> Lucian Adrian Grijincu wrote:
>>> isn't this why they invented comments?
>>>
>>> why [...] when you can do:
>>> return x; //and place a witty comment here?
>>>
>>> saves a cmp and a jump.
>> Just forewarn - // isn't portable, please don't litter :) More than happy to see
>> /* */ style comments.
>
> Sorry for being ignorant :). I'll try to remember that.
I wouldn't have said anything if it wasn't the #2 most common mistake, right after
using tabs instead of spaces :) I know - MS spoils us by calling their C++ compiler
a C compiler ;-)
> I think someone with Netware experience should review this code.
Yes, if you weren't aware, patch is very tolerant of # comments, so you can attach
your patch with something like...
# Patch to clean up the code flow, but this needs review by a netware guru
# because...
#
Index: threadproc/netware/thread.c
===================================================================
--- threadproc/netware/thread.c (revision 584412)
+++ threadproc/netware/thread.c (working copy)
@@ -125,19 +125,14 @@
/* int *error */
...
which is not only clear to the reader, but applies without changes via patch.
Bill
Re: some simplifications for the Netware apr_thread_create code
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/14/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Lucian Adrian Grijincu wrote:
> > isn't this why they invented comments?
> >
> > why [...] when you can do:
> > return x; //and place a witty comment here?
> >
> > saves a cmp and a jump.
>
> Just forewarn - // isn't portable, please don't litter :) More than happy to see
> /* */ style comments.
>
Sorry for being ignorant :). I'll try to remember that.
> In your patch the adjustments are good at first glance, if none of the netware folks
> look at them I'll catch up soon. You might want to create an apr 'small code cleanups'
> tracking bug, and drop these in as (individual!) attachments for easy review.
>
http://issues.apache.org/bugzilla/show_bug.cgi?id=43620
But after looking at the code, I'm not whether I should attach the
patch or not. Here is why:
the code looks like this:
http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/netware/thread.c?view=markup
(*new)->ctx = NXContextAlloc( some_uninteresting_list_of_parameters );
stat = NXContextSetName( some_uninteresting_list_of_parameters );
stat = NXThreadCreate( some_uninteresting_list_of_parameters );
if(stat==0)
return APR_SUCCESS;
return(stat);// if error
Disclaimer: I have no idea what Netware is, does, eats, etc. I haven't
coded to it's API. But through a simple Google Search I've found that
when NXThreadCreate fails other projects free the context allocated at
the beginning of this little snip of code.
http://mail-archives.apache.org/mod_mbox/httpd-cvs/200110.mbox/%3C20011016232705.3884.qmail@icarus.apache.org%3E
search in the page when it mentions NXThreadCreate.
This looks as a better approach.
(*new)->ctx = NXContextAlloc( some_uninteresting_list_of_parameters );
stat = NXContextSetName( some_uninteresting_list_of_parameters );
stat = NXThreadCreate( some_uninteresting_list_of_parameters );
if(stat==0)
return APR_SUCCESS;
NXContextFree((*new)->ctx); /*Free your resources*/
return(stat);
The internal pool could be freed before the return as well (even
though it will get freed later by the parent pool).
Also this context should be freed in other functions as well (e.g.
apr_thread_exit).
I think someone with Netware experience should review this code.
--
Lucian
Re: some simplifications for the Netware apr_thread_create code
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
> isn't this why they invented comments?
>
> why [...] when you can do:
> return x; //and place a witty comment here?
>
> saves a cmp and a jump.
Just forewarn - // isn't portable, please don't litter :) More than happy to see
/* */ style comments.
In your patch the adjustments are good at first glance, if none of the netware folks
look at them I'll catch up soon. You might want to create an apr 'small code cleanups'
tracking bug, and drop these in as (individual!) attachments for easy review.
>> The compiler can't care less if you compare (rv == 0) or (!rv),
>> it optimizes it the same way.
>
> What !rv? i don't wanna do the comparison at all.
:)
Re: some simplifications for the Netware apr_thread_create code
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/14/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Lucian Adrian Grijincu wrote:
> > Just removing some unused code.
> > APR_SUCCESS is deemed to be zero as I understand it, so no use in
> > checking for it explicitly.
>
> Untrue, because it makes things clearer to the reader.
>
isn't this why they invented comments?
why do:
if(x==0)
return 0;
return x;
when you can do:
return x; //and place a witty comment here?
saves a cmp and a jump.
> The compiler can't care less if you compare (rv == 0) or (!rv),
> it optimizes it the same way.
>
What !rv? i don't wanna do the comparison at all.
--
Lucian
Re: some simplifications for the Netware apr_thread_create code
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
> Just removing some unused code.
> APR_SUCCESS is deemed to be zero as I understand it, so no use in
> checking for it explicitly.
Untrue, because it makes things clearer to the reader.
The compiler can't care less if you compare (rv == 0) or (!rv),
it optimizes it the same way.
Bill