opencog-dev team mailing list archive

["David Hart" <hart@xxxxxxxxxxxx>]

[NB: moved thread to opencog-dev]

Okay, so http://bazaar.launchpad.net/~qg/opencog-framework/dev/revision/774needs
some work (breaking out into smaller safer patches) and
http://bazaar.launchpad.net/~qg/opencog-framework/dev/revision/775 is really
on the larger whitespace issue.

OTOH http://bazaar.launchpad.net/~qg/opencog-framework/dev/revision/773 and
http://bazaar.launchpad.net/~qg/opencog-framework/dev/revision/776 look
small and safe (and no whitespace issues). okay to merge?

-dave

On Thu, May 29, 2008 at 3:43 PM, Linas Vepstas <linasvepstas@xxxxxxxxx>
wrote:

>
> 2008/5/28 Trent Waddington <trent.waddington@xxxxxxxxx>:
> >
> > My development tree now has a working port of main:
> >
> >    bzr branch lp:~qg/opencog-framework/dev
> >
> >    or see https://code.launchpad.net/~qg/opencog-framework/dev<https://code.launchpad.net/%7Eqg/opencog-framework/dev>
>
> Woof!
>
> This is an absolutely HUGE patch. Surely it can be broken
> down into a patch series, no?  now that I;ve looked at it,
> I see at least five patches:
> -- whitespcae changes
> -- floating point casting
> -- boolean casting
> -- addition of platform.h everywhere
> -- assorted win32 changes
>
> I don't think we've established ground rules for patches yet,
> but normal coding standards as I know them says this patch
> is too big to be easily reviewed.
>
> If I start digging a little bit:, I see stuff like this:
>
>  return flags & MARKED_FOR_REMOVAL;     589             return flags &
> MARKED_FOR_REMOVAL ? true : false;
> 589     }       590     }
>
> (Whoof. is there a way of making launchpad show
> the actual diff? I'm looking at
>
> http://bazaar.launchpad.net/~qg/opencog-framework/dev/revision/774<http://bazaar.launchpad.net/%7Eqg/opencog-framework/dev/revision/774>
> Maybe my account settings are wrong, but  what
> is displayed for me is this nasty-assed peice of
> color coded whack-job that is three times wider than my
> computer screen,  and cannot be cut and pasted into
> email... hork.  .)
>
> Anyway, are you really saying that microsoft compilers
> are unable to compile this code? Is this change really
> necesssary? It wreaks the readability of the code,
> and is just begging for programmer error .
>
> I also see things like
> 341         cassert(TRACE_INFO, boost::get<Handle>(&*it),
> "AtomSpace::addAtom(): Vertex should be of 'Handle' type.");    341
> cassert(TRACE_INFO, boost::get<Handle>(&*it) != 0,
> "AtomSpace::addAtom(): Vertex should be of 'Handle' type.");
> 342             342
>
> which appears to be another gratuitous change that
> is seems rather inappropriate.
>
>  170        char* tvToken = __strtok_r(s, "{}", &buff);         175
> char* tvToken = __strtok_r(s, "{}", &buff);
>
> appears to have no change, other  than to introduce
> incorrect indentation.
>
> Either way, using double-underscore functions
> is incorrect; these are reseved in the C standard,
> we should be using the non-underscore vesion of this routine.
>
> 66      HandleMap<T>::~HandleMap<T>()   66      HandleMap<T>::~HandleMap()
>
> this is another whitepsace change.  I've nothing
> against whitespace changes, but these should
> be submitted as a separate patch, not merged
> with functional changes.
>
> 170         result->symmetric = s;      171             result->symmetric =
> (s != 0);
>
> This is a clear functional change, its not explained.
> It also introduces bad indentation.
>
> 46          cassert(TRACE_INFO, &name, "Node - name parameter should not
> be NULL.");     47          cassert(TRACE_INFO, &name != NULL, "Node - name
> parameter should not be NULL.");
>
> this is another gratuitous change.
>
>  char tvStr[length+1];  819         char *tvStr = new char[length+1];
>
> This is fundamentally wrong. the innitial string is allocated
> on stack, the changed version is on heap. stack allocation
> is much cheaper, it is done by the compiler, rather than at
> runtime.  stack allocs, esp for strings, should never be turned
> into mallocs.
>
> 123             return KKK*c/(1.0-c);   124             return
> (float)(KKK*c/(1.0-c));
>
> its already a float, it does not need to be cast. (it should not
> be cast, casting is bad, and should only be done when
> it can't be avoided.)
>
> 154         const char* nextToken = timeNodeName;       160         const
> char*
> nextToken = timeNodeName;
>
> whitespace?
>
> 52              static TruthValue* instance     = new
> SimpleTruthValue(MAX_TRUTH, MAX_TV_COUNT);      53              static
> TruthValue* instance     = new SimpleTruthValue((float)MAX_TRUTH,
> (float)MAX_TV_COUNT);
> 53              return *instance;       54              return *instance;
>
> This is wrong. If MAX_TRUTH has not been declared as
> a float,  it should be; this would fix any broken-ness
> everywhere, and not just here.
>
> 29              29
>                30      #ifdef WIN32
>                31      #include <TcpSocket.h>
>                32      #include <ISocketHandler.h>
>                33      #else
> 30      #include <Sockets/TcpSocket.h>  34      #include
> <Sockets/TcpSocket.h>
> 31      #include <Sockets/ISocketHandler.h>     35      #include
> <Sockets/ISocketHandler.h>
>                36      #endif
>
> Ehh? something wrong with the environment?
>
> 25      #include <exceptions.h>         25      #include <exceptions.h>
>                26      #ifdef WIN32
>                27      #include <ListenSocket.h>
>                28      #include <SocketHandler.h>
>                29      #else
> 26      #include <Sockets/ListenSocket.h>       30      #include
> <Sockets/ListenSocket.h>
> 27      #include <Sockets/SocketHandler.h>      31      #include
> <Sockets/SocketHandler.h>
>                32      #endif
>
> Ditto
> 112                 gettimeofday(&stv, NULL);   118
>  gettimeofday(&stv, NULL);
> 113                 gmtime_r(&(stv.tv_sec), &stm);      119
>       time_t t = stv.tv_sec;
>                120                 gmtime_r(&t, &stm);
>                121     #ifdef WIN32
>                122                             strftime(timestamp,
> sizeof(timestamp),
> "%Y-%m-%d %H:%M:%S", &stm);
>                123
>                124     #else
> 114                 strftime(timestamp, sizeof(timestamp), "%F %T",
> &stm);  125                 strftime(timestamp, sizeof(timestamp), "%F
> %T", &stm);
>                126     #endif
>
> the indentation is insane!
>
> Also, if we need to write portability routines for win32,
> we should write them.  Trying to hack something in like
> this on a case-by-case basis is inappropriate.
>
> 81        unsigned sleep(unsigned seconds);     91        unsigned
> sleep(unsigned seconds);
> 82              92              int gettimeofday(struct timeval* tp, void*
> tzp);
>
> More crazy indentation. I feel for you, I wish that opencog
> used tabs-only, since the four-spaces routine makes my
> thumbs tired. but since we live in four-spaces land, we
> have to stay there
>
> 83                      tc = (tcDecayRate * -new_tc) + ( (1.0-tcDecayRate)
> * old_tc);      83                      tc = (float)((tcDecayRate *
> -new_tc) +
> ( (1.0-tcDecayRate) * old_tc));
>
> what's with the cast ???  the result of multiplying a bunch
> of floats should eb afloat, there should be no need to cast!
>
> 94                      tc = (tcDecayRate * new_tc) + ( (1.0-tcDecayRate)
> * old_tc);      94                      tc = (float)((tcDecayRate * new_tc)
> +
> ( (1.0-tcDecayRate) * old_tc));
>
> ditto
>
> 109                 tc = (tcDecayRate * new_tc) + ( (1.0-tcDecayRate) *
> old_tc);        109                 tc = (float)((tcDecayRate * new_tc) + (
> (1.0-tcDecayRate) * old_tc));
>
> ditto.,
>
> 39          genPatternDensity = HDEMO_DEFAULT_PATTERN_DENSITY;  39
> genPatternDensity = (float)HDEMO_DEFAULT_PATTERN_DENSITY;
>
> this should be declared as a float, not cast to it.
> (the same way you changed 1.0 to 1.0f in other places.)
>
> 171                     hServer->density=atof(optarg); break;   171
>          hServer->density=(float)atof(optarg); break;
>
> atof already returns a double, it does not need to be cast
> to float!
>
> 120         tm=localtime(&tv.tv_sec);   125             time_t t =
> tv.tv_sec;
>                126         tm=localtime(&t);
>
> why?
>
> 132         importUpdateAgent = new ImportanceUpdatingAgent();  138
>   importUpdateAgent = new opencog::ImportanceUpdatingAgent();
>
> that's weird. Aren't we using opencog already?
>
> 124             SimpleTruthValue stv(0.0, 1.0);         125
> SimpleTruthValue stv(0.0, 1.0);
> 125             stv.setConfidence(0.01);        126
> stv.setConfidence(0.01f);
>
> I can see the point of adding f to 0.01, but why
> not also add it to 0.0 and 1.0? why not make
> this consistent everywhere?
>
> 147             SimpleTruthValue stv(strength, 1.0);    148
> SimpleTruthValue stv((float)strength, 1.0f);
>
> strength is already declared as a double,
> immediately above. Does not need a cast.
>
> 163             SimpleTruthValue stv(rank_sum, 1.0);    164
> SimpleTruthValue stv((float)rank_sum, 1.0f);
>
> ditto
> 137             SimpleTruthValue stv(sim,0.9);  138
> SimpleTruthValue stv((float)sim,0.9f);
> 138             return stv;     139             return stv;
>
> ditto.
>
> 501                             qname.insert(0,1,'\'');         502
>            qname.insert((size_t)0,(size_t)1,'\'');
>
> ehh? what's this?
>
> 605             return local_id_cache.count(h);         606
> return
> local_id_cache.count(h) != 0;
>
> why?
>
> 698             SimpleTruthValue stv(rp.mean, rp.count);        699
> SimpleTruthValue stv((float)rp.mean, (float)rp.count);
>
> more pointless casts. I don't understnad this obsession
> with casting. casting is bad.
>
>                17      #ifdef WIN32
>                18      #define dbgprt printf
>                19      #else
>
> I'm pretty sure you don't want to do this, this will
> turn on debuggin printfs permanently for windows.
>
> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google Groups
> "OpenCog.org (Open Cognition Project)" group.
> To post to this group, send email to opencog@xxxxxxxxxxxxxxxx
> To unsubscribe from this group, send email to
> opencog-unsubscribe@xxxxxxxxxxxxxxxx
> For more options, visit this group at
> http://groups.google.com/group/opencog?hl=en
> -~----------~----~----~----~------~----~------~--~---
>
>