kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

>From what little I saw (which was too much), fixing the polygon library
is not going to be trivial.  I know the CERN folks are going to work on
improving the polygon code so maybe I can steer them to fix this kind
ugliness before adding any new code.  No hurry, it's probably been like
this since the polygon library was added to kicad.

On 9/28/2016 8:58 PM, Chris Pavlina wrote:
> The whole thing gives me shivers. I'd much rather rewrite the code to
> not do this at all, but if we're going to try to "fix" it "in place",
> I'd much rather keep the comparison in one documented location than
> scatter them around. It will be quite unobvious later why we were
> dancing around casting things in weird ways otherwise.
> 
> I won't have time to commit it myself for a little while, though.
> 
> On Wed, Sep 28, 2016 at 08:55:41PM -0400, Wayne Stambaugh wrote:
>> Ewww!  Gives me shivers but it drives the point home until it can be
>> fixed properly.  I'm fine with you committing this until then.
>>
>> On 9/28/2016 8:31 PM, Chris Pavlina wrote:
>>> Declaring the "unsigned long long" volatile should be (and is, I tested)
>>> sufficient to force the compiler to actually read that integer at
>>> runtime and not trust that it came from a cast pointer.
>>>
>>> "unsigned long long" is horribly ugly though - surely we have uintptr_t
>>> in cstdint on all our supported platforms? It's _meant_ for this, though
>>> technically optional.
>>>
>>> Also, if we were to do this, I'd be strongly in favor of factoring it
>>> out into a function with some explanation as to why. Something like:
>>>
>>> /* Some very bad people have written code to test whether a reference is
>>>  * NULL. As permitted by standard, the compiler optimizes this out to
>>>  * punish these very bad people. This function forces it to perform the
>>>  * test, as an alternative to rewriting the horrors contained below.
>>>  */
>>>
>>> template<typename T>
>>> bool ref_is_null( T& x )
>>> {
>>>     volatile uintptr_t px = (uintptr_t) &x;
>>>     return px == 0;
>>> }
>>>
>>> On Thu, Sep 29, 2016 at 10:14:47AM +1000, Cirilo Bernardo wrote:
>>>> Just in case NULL is indeed deliberately cast to a ref (as in the case of
>>>> OpenCascade), how about  replacing this test with something like:
>>>>
>>>> void foo(int &i)
>>>> {
>>>>     unsigned long long z = (unsigned long long) &i;
>>>>
>>>>     if( z < 2 )
>>>>         std::cout << "blah";
>>>> }
>>>>
>>>> Of course if the compiler is *too clever* it will realize that "2" is
>>>> still an invalid
>>>> pointer on 32 and 64-bit addressing schemes and still optimize this out, but
>>>> my gcc 6.2.0 isn't that clever. The only *other* problem is that
>>>> unsigned long long
>>>> is to be at least 64b but I have no idea if there is anything in the language
>>>> specification to guarantee it is at least as large as a pointer type.
>>>>
>>>> - Cirilo
>>>>
>>>>
>>>>
>>>> On Thu, Sep 29, 2016 at 1:57 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>>>>> I didn't test what the compiler generated but I suspected that was the
>>>>> case from warning message. If that's the case then I will apply the
>>>>> patch but it does mask the underlying issue which could fail spectacularly.
>>>>>
>>>>> On 9/28/2016 11:43 AM, Chris Pavlina wrote:
>>>>>> Well, as for removing them being a good or bad idea - the compiler is
>>>>>> probably doing that anyway. I just tested on GCC, at -O1 or greater it
>>>>>> just assumes all references are non-NULL and replaces any "&ref != 0"
>>>>>> with a hard-coded boolean 'true'. So... there's a pretty good chance we
>>>>>> haven't had those checks for quite some time.
>>>>>>
>>>>>> On Wed, Sep 28, 2016 at 09:24:40AM -0400, Wayne Stambaugh wrote:
>>>>>>> I normally don't like to rant but OMFG!  I just looked at the code path
>>>>>>> that lead to this wonderful piece of code and I'm now regretting that.
>>>>>>> You just cannot unsee that.  See if you can follow the logic (assuming
>>>>>>> logic was actually used to write this code) backwards from
>>>>>>> Triangle::NeighborAcross().  I'm not sure removing the invalid null
>>>>>>> reference checks is a good idea even though I've seen the compiler
>>>>>>> complain about it and agree with it.  In what universe do you blindly
>>>>>>> convert a potentially NULL pointer into a C++ reference, pass it to the
>>>>>>> caller, and go merrily on you way?  This really needs to be rewritten
>>>>>>> using pointers with the appropriate NULL pointer checks along the way.
>>>>>>> It's hard to imagine this ever worked properly.
>>>>>>>
>>>>>>> On 9/27/2016 1:33 PM, Simon Richter wrote:
>>>>>>>>
>>>>>>>> Triangle::NeighborAcross returns a reference, which must refer to a valid
>>>>>>>> object whose address cannot be 0. Thus, this test is nonsensical.
>>>>>>>> ---
>>>>>>>>  polygon/poly2tri/sweep/sweep.cc | 16 ----------------
>>>>>>>>  1 file changed, 16 deletions(-)
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>
>>>>> _______________________________________________
>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>> More help   : https://help.launchpad.net/ListHelp
>>