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

Guillaume Papin guillaume.papin at epitech.eu
Thu Aug 8 08:38:42 PDT 2013

  Note that I added an overload for the `addAngledInclude()` function to take a FileEntry instead of a StringRef. The transform I'm working on is using this.

  I'm okay to put this class in a common place such as tooling so that clang-tidy can benefit from its features too.
  For the moment it fits my needs but I think it should be improved before it's 'widely available'.

  There is one case I think it would be nice to handle but I haven't found yet how to to it. "X Macro" headers. Such as `clang/AST/BuiltinTypes.def` which appears in the middle of the code.

    class BuiltinType : public Type {
      enum Kind {
    #define BUILTIN_TYPE(Id, SingletonId) Id,
    #define LAST_BUILTIN_TYPE(Id) LastKind = Id
    #include "clang/AST/BuiltinTypes.def"

  With the current code an include will be inserted after this one, which is not desired.

  And I realize just now something else is missing. If a header with a header guard doesn't contain any include, the include will be inserted before the header guard. This should make the code invalid but it's probably not something desired.

    /// This is a file header

    #include <foo> // insertion is done here

    #ifndef GUARD_H
    #define GUARD_H

    // here would be a better place for including <foo>

    #endif // GUARD_H

Comment at: cpp11-migrate/Core/IncludeDirectives.h:11
@@ +10,3 @@
+/// \file
+/// \brief This file declares the IncludeDirectives class that helps detecting
+/// and modifying \#include directives.
Edwin Vane wrote:
> ...helps **with** detecting...

Comment at: cpp11-migrate/Core/IncludeDirectives.h:66
@@ +65,3 @@
+    clang::SourceLocation Loc;
+    const clang::FileEntry *File;
+    bool Angled;
Edwin Vane wrote:
> Is this member necessary? Scanning the uses of this class it seems this information is already available elsewhere.
Yes it's necessary but I guess the name wasn't clear. I renamed it to `getIncludedFile()`.
This is the file being included, for `#include <iostream>` it will be the FileEntry for `iostream`.

Comment at: cpp11-migrate/Core/IncludeDirectives.h:82
@@ +81,3 @@
+  typedef llvm::SmallPtrSet<const clang::FileEntry *, 32> SeenFilesSet;
+  bool lookForInclude(const clang::FileEntry *File, llvm::StringRef Include,
+                      const LocationVec &Locations, SeenFilesSet &Seen) const;
Edwin Vane wrote:
> I know private member functions don't require documentation but a little bit might help with these functions since their parameter lists are non-trivial.
Done. Thanks, it helps me figuring out one of the parameter wasn't used.
Some of these functions had some non-doxygen documentation that was in the source file. I moved them to the header and made them doxygen-friendly.

I also made `hasInclude()` public so the user can query the class about that.

Totally unrelated but I discovered this by chance. In Phabricator you can comment a range of lines by holding the mouse button pressed while going through the lines you want to comment.

Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:1
@@ +1,2 @@
+//===-- Core/IncludeDirectives.h - Include directives handling ------------===//
Edwin Vane wrote:
> Nit: this is the .cpp file not .h.

Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:28
@@ +27,3 @@
+/// \c IncludeDirectives.
+class IncludeDirectivesPPCallback : public clang::PPCallbacks {
Edwin Vane wrote:
> Any reason this callback class isn't also in the anonymous namespace?
Yes, this class is friend with IncludeDirectives.

Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:52
@@ +51,3 @@
+    : CI(CI), Sources(CI.getSourceManager()) {
+  CI.getPreprocessor().addPPCallbacks(new IncludeDirectivesPPCallback(this));
Edwin Vane wrote:
> Maybe a quick comment to indicate addPPCallbacks() takes ownership of the provided callback.
Good point. I learned this the hard way.

Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:55
@@ +54,3 @@
+namespace {
+// Guess the end-of-line sequence used in the given FileID. If the sequence
Edwin Vane wrote:
> Maybe it's just me but I like all the anonymous namespace stuff at the top of the file and not interspersed between the implementations of member functions for a class.
I try to put 'utility' functions close to functions that use them but I have no strong opinion about this.
I read the coding standard section about anonymous namespace (http://llvm.org/docs/CodingStandards.html#anonymous-namespaces). So I removed some of the anonymous namespace (using static in this case).

I also put these 'utility' stuff on the top of the file.

Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:62
@@ +61,3 @@
+  StringRef Buffer;
+  if (!Invalid)
+    Buffer = Content.substr(Content.find_first_of("\r\n"));
Edwin Vane wrote:
> I would just `assert(!Invalid)`.
The `Invalid` argument being optional I decided to skip the check entirely.

Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:233
@@ +232,3 @@
+  Token Tok;
+  for (;;) {
+    // find beginning of end-of-line sequence
Edwin Vane wrote:
> Maybe a comment about what this for loop is trying to accomplish. Some examples would be nice.
A comment was a bit before the `for(;;)`, I tried to make this better let me know if it's okay. FYI an unit test tests exactly this case.
I changed a bit the way the loop is written, it should be slightly easier to understand now.

Comment at: unittests/cpp11-migrate/IncludeDirectivesTest.cpp:165
@@ +164,3 @@
+TEST(IncludeDirectivesTest, indirectIncludes) {
+      "#include </virtual/foo.h>\n",
Edwin Vane wrote:
> Isnt' this test the same as avoidDuplicates?
avoidDuplicates checks that we will not insert the same include twice in the same file.

  #include <foo>
  #include <foo>  // avoids this

This test goes a bit further and tests that we won't insert an include if it is included 'indirectly' by one of the include.

  #include <foo-inner>

  #include <a.h>
  #include <foo-inner> // avoids this

It also tests situation like this:

  #include <foo>

  #include <bar>
  // test that `<foo>` will be added in `b.h` even if `<foo>` is included
  // in `a.h` which is included before `b.h` in `a.c`.
  #include <foo>

  #include <a.h>
  #include <b.h>


More information about the cfe-commits mailing list