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

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added a comment.

Thank you for your comments! I've addressed many of them and now address the rest.



================
Comment at: clang/lib/Format/Format.cpp:2759
+          IncludeName =
+              StringRef(Twine("<", IncludeName).concat(Twine(">")).str());
+        }
----------------
curdeius wrote:
> HazardyKnusperkeks wrote:
> > How does that not go out of scope and I have a dangling reference?
> Why not doing it the other way round, i.e. trimming `<>"` from include name?
> Why not doing it the other way round, i.e. trimming `<>"` from include name?

Because the include name is used for sorting later and if the `<>` is removed from the name, the `<>` included will be mixed with the `""`. That's undesirable. 


================
Comment at: clang/lib/Format/Format.cpp:2759
+          IncludeName =
+              StringRef(Twine("<", IncludeName).concat(Twine(">")).str());
+        }
----------------
kwk wrote:
> curdeius wrote:
> > HazardyKnusperkeks wrote:
> > > How does that not go out of scope and I have a dangling reference?
> > Why not doing it the other way round, i.e. trimming `<>"` from include name?
> > Why not doing it the other way round, i.e. trimming `<>"` from include name?
> 
> Because the include name is used for sorting later and if the `<>` is removed from the name, the `<>` included will be mixed with the `""`. That's undesirable. 
> How does that not go out of scope and I have a dangling reference?

Sorry I was a bit lost with this but with some help I found other places in which `concat` was used and `toStringRef`. I hope everything is fine now.


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:177
 const char IncludeRegexPattern[] =
-    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+    R"(^[\t\ ]*[@#]?[\t\ ]*(import|include)[^"<]*[\t\n\ \\]*("[^"]+"|<[^>]+>|[^"<>;]+;))";
 
----------------
HazardyKnusperkeks wrote:
> HazardyKnusperkeks wrote:
> > I don't see why this is needed. It should match with the previous stuff.
> Do we want that optional?
> I don't see why this is needed. It should match with the previous stuff.

That's why I thought too. I swear I ran the tests before and I wouldn't work without this. But I re-ran the tests and now it works. Thanks for bringing up this very obvious part.


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:177
 const char IncludeRegexPattern[] =
-    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+    R"(^[\t\ ]*[@#]?[\t\ ]*(import|include)[^"<]*[\t\n\ \\]*("[^"]+"|<[^>]+>|[^"<>;]+;))";
 
----------------
kwk wrote:
> HazardyKnusperkeks wrote:
> > HazardyKnusperkeks wrote:
> > > I don't see why this is needed. It should match with the previous stuff.
> > Do we want that optional?
> > I don't see why this is needed. It should match with the previous stuff.
> 
> That's why I thought too. I swear I ran the tests before and I wouldn't work without this. But I re-ran the tests and now it works. Thanks for bringing up this very obvious part.
> Do we want that optional?

We don't need it optional for the tests to pass. Only for C++20 Modules, we need it optional. But this is *not* part of this change. I've removed the optional part of this. 


================
Comment at: clang/unittests/Format/SortIncludesTest.cpp:471
+                 "@import Foundation;\n"
+                 "#import \"a.h\"\n"));
+}
----------------
HazardyKnusperkeks wrote:
> I've no idea about obj-c, is it feasible to mix `#` and `@`? If so please add a test where the `@` is between `#`s.
> I've no idea about obj-c, is it feasible to mix `#` and `@`? If so please add a test where the `@` is between `#`s.

This test is copied from the issue itself. I have no idea about Obj-c as well. The `@` already *is* between the `#`. Did I miss something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121370