[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 {
public:
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...
Fixed.
================
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.
Fixed.
================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:28
@@ +27,3 @@
+/// \c IncludeDirectives.
+class IncludeDirectivesPPCallback : public clang::PPCallbacks {
+public:
----------------
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) {
+ EXPECT_EQ(
+ "#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.
a.h
#include <foo-inner>
a.c
#include <a.h>
#include <foo-inner> // avoids this
It also tests situation like this:
a.h
#include <foo>
b.h
#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>
a.c
#include <a.h>
#include <b.h>
http://llvm-reviews.chandlerc.com/D1287
More information about the cfe-commits
mailing list