Stephen requested review of this revision. Stephen added a comment. Thanks for the review! I think I've addressed the comments -- hopefully I kept it inline with what you're thinking. Please have another look at your leisure.
================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:82 + const auto IsSizeTypeOrDifferenceType = hasType( + namedDecl(anyOf(hasName("size_type"), hasName("difference_type")))); + ---------------- aaron.ballman wrote: > Should this also support `size_t` and `ptrdiff_t` given that those are > usually the same as `size_type` and `difference_type`? Thanks, yeah, that's a good idea. I was somewhat apprehensive to ignore all conversions involving size_t, but it's probably ok. Now that we are, we don't need the ::size() method stuff, so I removed that. And it seemed like I might as well make the list of types configurable, so I rolled that in here too. So it's a bigger change that you maybe had intended, though hopefully not too far off the mark. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-off.cpp:1 +// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \ +// RUN: -config="{CheckOptions: [ \ ---------------- hokein wrote: > instead of having multiple individual test files, I think we can group them > into a single file, you can use the flag `-check-suffix=`, like > > ``` > // RUN: %check_clang_tidy -check-suffix=TESTCASE1 ... > // RUN: %check_clang_tidy -check-suffix=TESTCASE2 ... > > > // some warning code for test-case1 > CHECK-MESSAGES-TESTCASE1: .... > ``` Thanks! Besides cutting down the number of files, it really helped clarify the distinction in behavior with the warning on/off. I coalesced the on/off pairs into a single, but kept the options tested in separate files, as that seemed to be the pattern in place. Let me know if you'd prefer to squeeze them into fewer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99543/new/ https://reviews.llvm.org/D99543 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits