[PATCH] cpp11-migrate: Add a class to support include directives modifications

[Edwin Vane]

  As for umbrella headers, not sure they're really an issue. Such headers don't contain any code and so you'd never find yourself adding an include to such a file since there's no code to transform.


================
Comment at: unittests/cpp11-migrate/IncludeDirectivesTest.cpp:165-167
@@ +164,5 @@
+TEST(IncludeDirectivesTest, indirectIncludes) {
+  EXPECT_EQ(
+      "#include </virtual/foo.h>\n",
+      addIncludeInCode("/virtual/foo-inner.h", "#include </virtual/foo.h>\n"));
+
----------------
Guillaume Papin wrote:
> Edwin Vane wrote:
> > I'm still not convinced this EXPECT is testing what you think. I understand the point of this test is to test indirect includes but this particular EXPECT is not using `makeIndirectTestsAction()` so as far as I see, it's identical to the avoidDuplicates() test. What am I missing?
> I can put this test in `avoidDuplicates()` if you want?
> 
> The difference (which is notable in the algorithm) is that the include is not directly in the file we are checking. So in the algorithm I forgot to check the includes recursively (or if it's badly implemented) removing this test will make the issue unnoticeable (I just tried to comment the part of the code and only this test detects it).
I see my problem now. Can you please add a comment here explaining that foo.h includes foo-inner.h as set up by TestAddIncludeAction?

================
Comment at: cpp11-migrate/Core/IncludeDirectives.h:58
@@ +57,3 @@
+private:
+  friend class IncludeDirectivesPPCallback;
+
----------------
Guillaume Papin wrote:
> Edwin Vane wrote:
> > Why do we even need friendship here? I know you have `InclusionDirective()` as private but according to docs, this is actually a public member of the clang::PPCallbacks class. You should probably also make it public. At this point, friendship is no longer necessary and then you can put the callbacks class into an anonymous namespace.
> Maybe you got the friendship in the wrong order? The friendship is used so that the callback can fill-in the IncludeDirectives data.
> The visibility of `InclusionDirective()` doesn't matter, it will be found by the PPCallbacks interface but I there is no need to access this member directly.
You're right of course. I missed the fact the callbacks actually set data in the other class. Generally I try to avoid friend classes but I think this is a case of intentional strong coupling between the two.


http://llvm-reviews.chandlerc.com/D1287