[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

Thu Apr 18 17:36:39 PDT 2019

hfinkel added a comment.

In D59712#1472544 <https://reviews.llvm.org/D59712#1472544>, @jdenny wrote:

> In D59712#1472358 <https://reviews.llvm.org/D59712#1472358>, @hfinkel wrote:
>
> > > I've never tried running the other tests you mention, for any patch.  I thought people normally left those to the bots.  Should this patch be handled differently?
> >
> > We have a lot of people actively working off of trunk, and we try very hard to keep trunk clean. The bots are a second line of defense, not the primary checkers. In any case, this comes down to professional judgement. It is not uncommon to ask for a patch author to check self hosting and a test suite run before committing - specifically, those patches that might affect correctness, or introduce other subtle problems, and for which running the compiler over a bunch of C/C++ code might uncover a problem.
>
>
> Thanks for explaining.  It's my first time receiving these particular requests (probably because of what parts of LLVM I normally edit), so I wasn't sure I understood.

No problem.

> For self-hosting, is it best to build again with CMAKE_C_COMPILER and CMAKE_CXX_COMPILE pointing into the previous build, or is there a better approach?

That's what I do.

> 
> 
>> Also, is this review now missing some files? I see here only updates to  APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. Nothing that would cause changes to the tests, however (maybe I'm just missing something).
> 
> All looks fine to me.  The APSInt.h changes are the reason for all the test changes.

Indeed. I forgot that you were changing overrides.

Thanks!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712