[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D88640#2339349 , @rupprecht wrote:

> It looks like this fix caused a different regression in not accepting 
> `name..h` as the main header for `name..cc`, e.g.:
>
>   $ cat /tmp/foo.bar.cc
>   #include "a.h"
>   #include "z.h"
>   #include "foo.bar.h"
>   
>   $ clang-format /tmp/foo.bar.cc  # Before
>   #include "foo.bar.h"
>   
>   #include "a.h"
>   #include "z.h"
>   
>   $ clang-format /tmp/foo.bar.cc  # After
>   #include "a.h"
>   #include "foo.bar.h"
>   #include "z.h"

oh, sorry for the regression. Will try to fix that, thanks for the example!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

It looks like this fix caused a different regression in not accepting 
`name..h` as the main header for `name..cc`, e.g.:

  $ cat /tmp/foo.bar.cc
  #include "a.h"
  #include "z.h"
  #include "foo.bar.h"
  
  $ clang-format /tmp/foo.bar.cc  # Before
  #include "foo.bar.h"
  
  #include "a.h"
  #include "z.h"
  
  $ clang-format /tmp/foo.bar.cc  # After
  #include "a.h"
  #include "foo.bar.h"
  #include "z.h"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Haojian Wu via Phabricator via cfe-commits]

hokein closed this revision.
hokein marked an inline comment as done.
hokein added a comment.

committed in c1b209cc61290f1ce1243470b825e0994645cb7d 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:237
+
+  StringRef HeaderStem =
+  llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(

this deserves a comment: Not matchingStem: implementation files may have 
compound extensions but headers may not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added a project: clang.
hokein requested review of this revision.

We receive internal bugs about this false positives after D86597 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88640

Files:
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/unittests/Format/SortIncludesTest.cpp


Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -151,6 +151,16 @@
   EXPECT_TRUE(sortIncludes(FmtStyle, Code, GetCodeRange(Code), 
"a.cc").empty());
 }
 
+TEST_F(SortIncludesTest, NoMainFileHeader) {
+  std::string Code = "#include \n"
+ "\n"
+ "#include \"a/extra_action.proto.h\"\n";
+  FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
+  EXPECT_TRUE(
+  sortIncludes(FmtStyle, Code, GetCodeRange(Code), "a/extra_action.cc")
+  .empty());
+}
+
 TEST_F(SortIncludesTest, SortedIncludesInMultipleBlocksAreMerged) {
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
   EXPECT_EQ("#include \"a.h\"\n"
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -233,7 +233,10 @@
 bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const {
   if (!IncludeName.startswith("\""))
 return false;
-  StringRef HeaderStem = matchingStem(IncludeName.drop_front(1).drop_back(1));
+
+  StringRef HeaderStem =
+  llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(
+  1) /* remove the surrounding "" or <> */);
   if (FileStem.startswith(HeaderStem) ||
   FileStem.startswith_lower(HeaderStem)) {
 llvm::Regex MainIncludeRegex(HeaderStem.str() + Style.IncludeIsMainRegex,


Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -151,6 +151,16 @@
   EXPECT_TRUE(sortIncludes(FmtStyle, Code, GetCodeRange(Code), "a.cc").empty());
 }
 
+TEST_F(SortIncludesTest, NoMainFileHeader) {
+  std::string Code = "#include \n"
+ "\n"
+ "#include \"a/extra_action.proto.h\"\n";
+  FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
+  EXPECT_TRUE(
+  sortIncludes(FmtStyle, Code, GetCodeRange(Code), "a/extra_action.cc")
+  .empty());
+}
+
 TEST_F(SortIncludesTest, SortedIncludesInMultipleBlocksAreMerged) {
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
   EXPECT_EQ("#include \"a.h\"\n"
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -233,7 +233,10 @@
 bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const {
   if (!IncludeName.startswith("\""))
 return false;
-  StringRef HeaderStem = matchingStem(IncludeName.drop_front(1).drop_back(1));
+
+  StringRef HeaderStem =
+  llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(
+  1) /* remove the surrounding "" or <> */);
   if (FileStem.startswith(HeaderStem) ||
   FileStem.startswith_lower(HeaderStem)) {
 llvm::Regex MainIncludeRegex(HeaderStem.str() + Style.IncludeIsMainRegex,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits