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