[PATCH] D138797: [include-cleaner] Implement IWYU begin_keep/end_keep pragma support.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:312
+
+  size_t offsetToLineNum(llvm::Annotations MainFile, size_t Offset) {
+    int Count = MainFile.code().substr(0, Offset).count('\n');
----------------
this is only used in `IWYUKeep` test, I'd make it as a local lambda (with MainFile captured) in the test:

```
auto OffsetToLineNum = [&MainFile](size_t Offset) { ... };
``` 


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:331
+
+    // IWYU pragma: begin_keep // Line 13
+    $keep3^#include "keep3.h"
----------------
I think we can also do similar thing on `$begin_keep_pragma^// IWYU pragma...`



================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:333
+    $keep3^#include "keep3.h"
+    $keep4^#include "keep4.h"
+    // IWYU pragma: end_keep
----------------
keep4.h can be removed to simplify the testcode.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:340
+    $keep6^#include "keep6.h"
+    $keep7^#include "keep7.h"
+    // IWYU pragma: end_keep
----------------
keep7.h can be removed.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:367
+
+  EXPECT_FALSE(PI.shouldKeep(18)); // line with "begin_keep" pragma
+  EXPECT_TRUE(
----------------
no need to repeatly test the begin_keep and end_keep pragmas.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138797/new/

https://reviews.llvm.org/D138797