[PATCH] D17484: [clang-tidy] introduce modernize-deprecated-headers check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for this check! Mostly looks good, but there are a number of style nits.

The most important question to this check is that the standard doesn't guarantee that the C++ headers declare all the same functions **in the global namespace** (at least, this is how I understand the section of the standard you quoted). This means that the check in its current form can break the code that uses library symbols from the global namespace. The check could add `std::` wherever it's needed (though it may be not completely trivial) to mitigate the risk. It's not what needs to be addressed in the first iteration, but it's definitely worth a look at in order to make the check actually useful. It also could be a separate check or a separate mode (configurable via parameters) of this check, while someone could have reasons to do the migration in two stages (add `std::` qualifiers and then switch to new headers).


================
Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:15
@@ +14,3 @@
+
+#include <iostream>
+#include <vector>
----------------
Is this header used?

================
Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:36
@@ +35,3 @@
+private:
+  std::map<std::string, std::string> CStyledHeaderToCxx;
+
----------------
There's a more effective container in LLVM: `llvm::StringMap<std::string>`. If your code guarantees that only string literals be used to initialize this map, this can be a `llvm::StringMap<StringRef>`.

================
Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:54
@@ +53,3 @@
+    : Check(Check), LangOpts(LangOpts) {
+  CStyledHeaderToCxx = {
+      {"assert.h", "cassert"},
----------------
It should be possible to initialize this in the constructor initializer list.

================
Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:77
@@ +76,3 @@
+
+  // Add C++ 11 headers
+  if (LangOpts.CPlusPlus11) {
----------------
nit: Please add a trailing period.

================
Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:94
@@ +93,3 @@
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  if (CStyledHeaderToCxx.count(std::string(FileName)) != 0) {
+    std::string Replacement =
----------------
1. s/`std::string(FileName)`/`FileName.str()`/
2. no need for this, if `llvm::StringMap` is used

================
Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:96
@@ +95,3 @@
+    std::string Replacement =
+        "<" + CStyledHeaderToCxx[std::string(FileName)] + ">";
+    Check.diag(FilenameRange.getBegin(), "including deprecated C++ header")
----------------
`FileName.str()`

================
Comment at: docs/clang-tidy/checks/modernize-deprecated-headers.rst:8
@@ +7,3 @@
+
+  Annex D (normative)
+  Compatibility features [depr]
----------------
This quote from the standard is too long for my taste. A simple reference to the relevant section should be enough (`[depr.c.headers]`).

================
Comment at: test/clang-tidy/modernize-deprecated-headers-cxx03.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s modernize-deprecated-headers %t -- -- -std=c++03 -isystem %S/Inputs/Headers
+
----------------
You seem to have forgotten to add these headers to `Inputs/Headers`. Or doesn't the check care about them actually being present?

================
Comment at: test/clang-tidy/modernize-deprecated-headers-cxx03.cpp:25
@@ +24,3 @@
+
+// Headers deprecated since C++11; expect no diagnostics
+#include <fenv.h>
----------------
Trailing period.

================
Comment at: test/clang-tidy/modernize-deprecated-headers-cxx03.cpp:32
@@ +31,3 @@
+
+// CHECK-FIXES: #include <cassert>
+// CHECK-FIXES-NEXT: #include <ccomplex>
----------------
Please also check diagnostic messages (`// CHECK-MESSAGES: ...`).


http://reviews.llvm.org/D17484