[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types
Hal Finkel via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
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.
> 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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits