[PATCH] [clang-tidy] Implement the include order checker for LLVM.

[Alexander Kornienko]

================
Comment at: clang-tidy/llvm/IncludeOrderCheck.cpp:30
@@ -27,4 +29,3 @@
                           const Module *Imported) override {
-    // FIXME: This is a dummy implementation to show how to get at preprocessor
-    // information. Implement a real include order check.
-    Check.diag(HashLoc, "This is an include");
+    // Report all includes in the main file.
+    if (SM.isInMainFile(HashLoc))
----------------
It would be useful to check the include order in headers too (except for system headers). The results from unwanted headers are filtered afterwards.

================
Comment at: clang-tidy/llvm/IncludeOrderCheck.cpp:54
@@ +53,3 @@
+
+static int getCategory(StringRef Filename, bool IsAngled, bool IsMainModule) {
+  // We leave the main module header at the top.
----------------
"Category" is too vague, a more specific name would be better. Maybe "sorting priority", or "precedence"?

================
Comment at: clang-tidy/llvm/IncludeOrderCheck.cpp:76
@@ +75,3 @@
+  while (Text[Offset] != '\0') {
+    // The end of a line can be of three forms: '\r' (old MacOS), '\n' (UNIX),
+    // or '\r\n' (DOS). Check for '\n' to cover UNIX and DOS style. or '\r'
----------------
The CR newline style seems to be old enough to be useless. And without supporting it std::strchr would do the job.



================
Comment at: clang-tidy/llvm/IncludeOrderCheck.cpp:101
@@ +100,3 @@
+
+  if (FirstTopLevelDecl.isInvalid())
+    return;
----------------
So, this won't work for files consisting only of #includes? That's sub-optimal.

================
Comment at: clang-tidy/llvm/IncludeOrderCheck.cpp:104
@@ +103,3 @@
+
+  // Purge includes after the first decl in the translation unit.
+  IncludeDirectives.erase(
----------------
How about a pure-lexer approach to finding the end of includes block? Just lex everything in between #include directives until you find a non-comment token that is not a part of a preprocessor directive?

BTW, is this needed at all if you only sort includes within each block?

================
Comment at: clang-tidy/llvm/IncludeOrderCheck.h:29
@@ +28,3 @@
+
+  void addIncludeDirective(SourceLocation Loc, CharSourceRange Range,
+                           StringRef Filename, bool IsAngled) {
----------------
Please move the function definition to the .cpp file.

================
Comment at: clang-tidy/llvm/IncludeOrderCheck.h:47
@@ +46,3 @@
+    bool IsAngled;         ///< true if this was an include with angle brackets
+    bool IsMainModule;     ///< true if this was the first include in a file
+  };
----------------
It's a bit confusing. What if the first include in a file was just an arbitrary header, which happened to be included first? What if the first include isn't special (e.g. we're analyzing a header file)?

Should we try to do filename matching?

http://reviews.llvm.org/D4741