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

Wed May 18 08:45:50 PDT 2022

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