[PATCH] D121370: [clang-format] SortIncludes should support "@import" lines in Objective-C

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk marked 3 inline comments as done.
kwk added inline comments.


================
Comment at: clang/lib/Format/Format.cpp:2692-2695
+  for (int i = Matches.size() - 1; i > 0; i--) {
+    if (!Matches[i].empty())
+      return Matches[i];
+  }
----------------
owenpan wrote:
> I think you missed `Matches[0]`.
I did miss it and fixed it but it doesn't make much sense to use the first match because that's always the overall string that's being matched. Nevertheless, I've used your porposed reverse loop and the logic is better if it includes even the first match.


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:176-188
 const char IncludeRegexPattern[] =
-    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+    R"(^[\t\ ]*[@#][\t\ ]*(import|include)([^"]*("[^"]+")|[^<]*(<[^>]+>)|[\t\ ]*([^;]+;)))";
+
+// Returns the last match group in the above regex (IncludeRegexPattern) that
+// is not empty.
+StringRef getIncludeNameFromMatches(const SmallVectorImpl<StringRef> &Matches) {
+  for (int i = Matches.size() - 1; i > 0; i--) {
----------------
owenpan wrote:
> owenpan wrote:
> > If these are the same as in `Format.cpp` above, we should move the definitions to `HeaderIncludes.h`.
> > If these are the same as in `Format.cpp` above, we should move the definitions to `HeaderIncludes.h`.
> 
> I meant we should remove the definitions from `Format.cpp` and add the declarations to `HeaderIncludes.h`.
I think I've done what you intended me to do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121370/new/

https://reviews.llvm.org/D121370