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

Edwin Vane edwin.vane at intel.com
Fri Aug 9 06:40:20 PDT 2013


  So do you have solutions for the problem of headers with header guards but no includes? 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.

  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.


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

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


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



More information about the cfe-commits mailing list