LegalizeAdulthood 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. ---------------- steakhal wrote: > 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? Nope, no insistence on my part. Just asking the question, which you've answered. I never even noticed the "stack" tab in phabricator before. Honestly, phabricator is not my favorite code reviewing tool, but it's what clang/llvm has chosen, so I live with it `:)` 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
