[petsc-dev] technical C question

[Matthew Knepley]

On Thu, Apr 7, 2016 at 7:15 PM, Barry Smith <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.

   Matt


>   Barry
>
>
>
> > On Apr 7, 2016, at 5:56 PM, Jed Brown <jed at jedbrown.org> wrote:
> >
> > Satish Balay <balay at mcs.anl.gov> writes:
> >
> >> On Thu, 7 Apr 2016, Jed Brown wrote:
> >>
> >>> Barry Smith <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20160407/2a9c0ac0/attachment.html>