[clang-tidy] modernize-deprecated-headers should ignore system headers

[steakhal]

Summary

The end-user has no way of 'fixing' bugs in the system library anyway.
Let's suppress these as well.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

37

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?

[steakhal]

Rebased after changing the name of the option.

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

37

D125209 should have included this file at clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

37

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.

[steakhal]

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

37

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?

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

37

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 :)

[steakhal]

Thanks for the review @LegalizeAdulthood!
Can I consider this change accepted?

[LegalizeAdulthood]

LGTM

clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp

void IncludeModernizePPCallbacks::InclusionDirective(
     StringRef SearchPath, StringRef RelativePath, const Module *Imported,
     SrcMgr::CharacteristicKind FileType) {
 
   // If we don't want to warn for non-main file reports and this is one, skip
   // it.
   if (!CheckHeaderFile && !SM.isInMainFile(HashLoc))
     return;
 
+  // Ignore system headers.
+  if (SM.isInSystemHeader(HashLoc))
+    return;
+
   // FIXME: Take care of library symbols from the global namespace.
   //
   // Reasonable options for the check:
   //
   // 1. Insert std prefix for every such symbol occurrence.
   // 2. Insert `using namespace std;` to the beginning of TU.
   // 3. Do nothing and let the user deal with the migration himself.
   SourceLocation DiagLoc = FilenameRange.getBegin();

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

 }
 
 #include <assert.h>
 // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
 
 #include <stdbool.h>
 // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers]
 
-#include <mysystemlib.h> // FIXME: We should have no warning into system headers.
-// CHECK-MESSAGES-CHECK-HEADER-FILE: 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.
 
 #include <mylib.h>
 // CHECK-MESSAGES-CHECK-HEADER-FILE: mylib.h:1:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
 
 namespace wrapping {
 extern "C" {
 #include <assert.h>  // no-warning
 #include <mylib.h>   // no-warning