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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
