[PATCH] D125770: [clang-tidy] modernize-deprecated-headers should ignore system headers

Wed May 18 08:35:01 PDT 2022

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