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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
