[petsc-dev] technical C question

[Jeff Hammond]

On Thursday, April 7, 2016, Matthew Knepley <knepley at gmail.com> wrote:

> On Thu, Apr 7, 2016 at 7:15 PM, Barry Smith <bsmith at mcs.anl.gov
> <javascript:_e(%7B%7D,'cvml','bsmith at mcs.anl.gov');>> wrote:
>
>>
>>    This works.
>>
>>  unsigned int joe = index%PETSC_BITS_PER_BYTE;
>>     if (joe > 7) joe = 7;
>>   return (BT_idx        = index/PETSC_BITS_PER_BYTE,
>>           BT_c          = array[BT_idx],
>>           BT_mask       = (char)(1 << joe),
>>
>> I think it is because clang is not smart enough to know that
>> (index%PETSC_BITS_PER_BYTE) is always between 0 and 7.
>>
>>    Hence it thinks that you might be doing something like    1 << 9
>> which is as I understand it undefined
>> http://stackoverflow.com/questions/21909412/why-would-the-outcome-of-this-shift-left-operation-be-deemed-undefined
>>
>>   So do we pander to the clang analyzer or just leave the warning? I vote
>> to leave the warning and not change the code.
>>
>
> I believe Bill also encouraged us toward clean code rather than 0
> warnings. I like this as well.
>
>
Some compilers allow you suppress warnings, potentially at file granularity
via pragmas...


>    Matt
>
>
>>   Barry
>>
>>
>>
>> > On Apr 7, 2016, at 5:56 PM, Jed Brown <jed at jedbrown.org
>> <javascript:_e(%7B%7D,'cvml','jed at jedbrown.org');>> wrote:
>> >
>> > Satish Balay <balay at mcs.anl.gov
>> <javascript:_e(%7B%7D,'cvml','balay at mcs.anl.gov');>> writes:
>> >
>> >> On Thu, 7 Apr 2016, Jed Brown wrote:
>> >>
>> >>> Barry Smith <bsmith at mcs.anl.gov
>> <javascript:_e(%7B%7D,'cvml','bsmith at mcs.anl.gov');>> writes:
>> >>>>   Should we caste to an unsigned PetscInt first then?
>> >>>
>> >>> It should be unsigned, yes.  Does that fix the warning?
>> >>
>> >> Nope..
>> >>
>> >> Commenting out the following line - or changing the argument thus is
>> >> making a difference.
>> >>
>> >> diff --git a/src/mat/impls/baij/seq/baijfact.c
>> b/src/mat/impls/baij/seq/baijfact.c
>> >> index fea37cb..e2c210c 100644
>> >> --- a/src/mat/impls/baij/seq/baijfact.c
>> >> +++ b/src/mat/impls/baij/seq/baijfact.c
>> >> @@ -1081,7 +1081,7 @@ PetscErrorCode MatICCFactorSymbolic_SeqBAIJ(Mat
>> fact,Mat A,IS perm,const MatFact
>> >>         ncols_upper++;
>> >>       }
>> >>     }
>> >> -    ierr =
>> PetscIncompleteLLAdd(ncols_upper,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr);
>> >> +    ierr =
>> PetscIncompleteLLAdd(ncols,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr);
>> >
>> > That looks like it changes the semantics.
>> >
>> > In any case, that file wasn't even mentioned in the message that Barry
>> > shared.  If it is indeed the same issue, then it would appear that the
>> > static analyzer has determined that it is possible for index to be
>> > negative.
>>
>>
>
>
> --
> What most experimenters take for granted before they begin their
> experiments is infinitely more interesting than any results to which their
> experiments lead.
> -- Norbert Wiener
>


-- 
Jeff Hammond
jeff.science at gmail.com
http://jeffhammond.github.io/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20160407/df00e009/attachment.html>