steakhal added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp:37 -#include <mysystemlib.h> // FIXME: We should have no warning into system headers. -// CHECK-MESSAGES-WARN-INTO-HEADERS: mysystemlib.h:1:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers] +#include <mysystemlib.h> // no-warning: Don't warn into system headers. ---------------- LegalizeAdulthood wrote: > steakhal wrote: > > LegalizeAdulthood wrote: > > > Where is this file? I can't find it in my tree. > > > > > > ``` > > > D:\legalize\llvm\llvm-project\clang-tools-extra > > > > dir/s/b mysystemlib.h > > > File Not Found > > > ``` > > > > > > Does this patch depend on some other unsubmitted patch? > > D125209 should have included this file at > > `clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h`. > OK, so it depends on D125209 .... why not just include these small changes in > that? > > Supposedly there is some way in phabricator to make one review dependent on > another, but instead of being an expert in phabricator this change seems > small enough to just fold into the other review, but perhaps opinions differ > on that. I set the 'parent revision', so the dependencies are present under the "Stack" tab. Sorry if I confused you. I still think it's a logically separate issue, which should be addressed in a separate commit. That change is already quite confusing. I already had to ever it once, thus I want to keep the cognitive complexity as low as possible there. Same applies here. Do you insist on merging these two? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125770/new/ https://reviews.llvm.org/D125770 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
