djasper added inline comments.
================
Comment at: lib/Format/Format.cpp:1411
@@ +1410,3 @@
+int getIncludePriority(const FormatStyle &Style,
+ SmallVector<llvm::Regex, 4> &CategoryRegexs,
+ StringRef IncludeName) {
----------------
But you should pass it as an ArrayRef or a SmallVectorImpl.
================
Comment at: lib/Format/Format.cpp:1413
@@ +1412,3 @@
+ StringRef IncludeName) {
+ for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
+ if (CategoryRegexs[i].match(IncludeName)) {
----------------
nit: no braces
================
Comment at: lib/Format/Format.cpp:1418
@@ +1417,3 @@
+ }
+ return -1;
+}
----------------
From the docs in Format.h: "If none of the regular expressions match, INT_MAX
is assigned". If that is true for an existing #include as well as the inserted
one, I don't see a reason, why those should belong to the same block. So, I
think you should return INT_MAX here and remove all the special-casing below.
================
Comment at: lib/Format/Format.cpp:1431
@@ +1430,3 @@
+
+ llvm::Regex IncludeRegex(
+ R"(^[\t\ ]*#[\t\ ]*include[\t\ ]*(["<][^">]*[">]))");
----------------
We already have an include regex in this file, I don't think we should have
two. Can you pull out the string into a constant?
================
Comment at: lib/Format/Format.cpp:1435
@@ +1434,3 @@
+ SmallVector<StringRef, 4> Matches;
+ // Create pre-compiled regular expressions for the #include categories.
+ SmallVector<llvm::Regex, 4> CategoryRegexs;
----------------
There is a lot of duplicated code between this and sortCppIncludes. Pull common
code out into functions.
================
Comment at: lib/Format/Format.cpp:1454
@@ +1453,3 @@
+ // extra category (priority 0) for main include.
+ std::vector<int> CategoryEndOffsets(MaxPriority + 1, -1);
+ bool MainIncludeFound = false;
----------------
There is no guarantee that people assign categories sequentially, I can easily
see people assigning category "1000" or "10000" to be able to add more
categories later. I think this should be a map and I think you then also don't
really need the vector. Also note that categories can be negative, which I
think would break this.
================
Comment at: lib/Format/Format.cpp:1458
@@ +1457,3 @@
+ int Offset = 0;
+ int AfterFirstDefine = 0;
+ SmallVector<StringRef, 32> Lines;
----------------
I am guessing that AfterFirstDefine is used so that we insert #includes after
header guards. I think we should name it AfterHeaderGuard than and either make
the behavior more strict or at least add a FIXME for it (only in header files,
only if the #define is after an #ifndef, ...)
================
Comment at: lib/Format/Format.cpp:1482
@@ +1481,3 @@
+ } else {
+ CategoryEndOffsets[Category] = Offset + Line.size() + 1;
+ }
----------------
I think if you assign the EndOffset of all lower categories that aren't set
yet, that will simplify the logic below.
================
Comment at: lib/Format/Format.cpp:1509
@@ +1508,3 @@
+ // Append the new #include after the corresponding category if the category
+ // exists. Otherwise, append after the last category that has higher
+ // priority than the current category. If there is no category, insert
after
----------------
What happens if there is no category with higher priority, but there is a
category with lower priority. Shouldn't you then insert the #include right
before that?
================
Comment at: lib/Format/Format.cpp:1548
@@ +1547,3 @@
+ const FormatStyle &Style) {
+ // FIXME: support other languages.
+ if (Style.Language != FormatStyle::LanguageKind::LK_Cpp)
----------------
I think you can remove this FIXME.. Seems obvious.
================
Comment at: lib/Format/Format.cpp:1551
@@ +1550,3 @@
+ return Replaces;
+ tooling::Replacements HeaderInsertionReplaces;
+ tooling::Replacements NewReplaces;
----------------
Why do you split out all the header insertion replacements here instead of just
ignoring the ones that aren't header insertions in fixCppIncludeInsertions?
================
Comment at: lib/Format/Format.cpp:1554
@@ +1553,3 @@
+ for (const auto &R : Replaces) {
+ if (R.getOffset() == -1U) {
+ HeaderInsertionReplaces.insert(R);
----------------
nit: No braces.
http://reviews.llvm.org/D20734
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
