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

[Guillaume Papin]

  > So do you have solutions for the problem of headers with header guards but no includes?
  I haven't look yet but while I was think about the X Macro problem I found this (http://clang.llvm.org/doxygen/classclang_1_1MultipleIncludeOpt.html#details) which I will look in more details if I can extract some interesting locations.

  > I'm fine documenting the X Macro header problem as a known shortcoming for now. I'd place a FIXME somewhere if there's an appropriate spot. Also log a bug in JIRA.

  Okay will do unless I manage to solve it in my next patch.

  > How easy is it to detect if a header doesn't have header guards? You could use that as a test to identify these X Macro headers which have the common feature of being intended for multiple inclusion. Once you can identify such inclusions you can disregard considering them as insertion points for new includes.

  Yeah I already looked at what could possibly help with this. I found two things that may be of interest:
  - for header guards: http://clang.llvm.org/doxygen/classclang_1_1MultipleIncludeOpt.html#details
  - for `#pragma once`: http://clang.llvm.org/doxygen/Pragma_8cpp_source.html#l00351

  But I believe there is at least another issue with umbrella headers that contains only includes (no guards):

  umbrella.h
    #include "foo.h"
    #include "bar.h"

  I haven't look at the modularize tool but I think they have to deal with this kind of things, maybe it's worth taking a look.


================
Comment at: cpp11-migrate/Core/IncludeDirectives.h:58
@@ +57,3 @@
+private:
+  friend class IncludeDirectivesPPCallback;
+
----------------
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.

================
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"));
+
----------------
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).


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