Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-format] Fix a regression in dumping the config #80628

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

owenca
Copy link
Contributor

@owenca owenca commented Feb 5, 2024

Commit d813af7 addressed a regression introduced by commit 3791b3f
but caused clang-format -dump-config to "hang".

This patch reverts changes to ClangFormat.cpp by both commits and reworks the cleanup.

Fixes #80621.

Commit d813af7 addressed a regression introduced by commit 3791b3f
but caused `clang-format -dump-config` to "hang".

This patch reverts changes to ClangFormat.cpp by both 3791b3f and
d813af7 and reworks the cleanup.

Fixes llvm#80621.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 5, 2024

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Commit d813af7 addressed a regression introduced by commit 3791b3f
but caused clang-format -dump-config to "hang".

This patch reverts changes to ClangFormat.cpp by both 3791b3f and
d813af7 and reworks the cleanup.

Fixes #80621.


Full diff: https://github.com/llvm/llvm-project/pull/80628.diff

1 Files Affected:

  • (modified) clang/tools/clang-format/ClangFormat.cpp (+25-24)
diff --git a/clang/tools/clang-format/ClangFormat.cpp b/clang/tools/clang-format/ClangFormat.cpp
index 5ee6092bb9bb7..a854a97a2d189 100644
--- a/clang/tools/clang-format/ClangFormat.cpp
+++ b/clang/tools/clang-format/ClangFormat.cpp
@@ -399,7 +399,8 @@ class ClangFormatDiagConsumer : public DiagnosticConsumer {
 };
 
 // Returns true on error.
-static bool format(StringRef FileName, bool IsSTDIN) {
+static bool format(StringRef FileName) {
+  const bool IsSTDIN = FileName == "-";
   if (!OutputXML && Inplace && IsSTDIN) {
     errs() << "error: cannot use -i when reading from stdin.\n";
     return false;
@@ -545,24 +546,25 @@ static void PrintVersion(raw_ostream &OS) {
 }
 
 // Dump the configuration.
-static int dumpConfig(bool IsSTDIN) {
+static int dumpConfig() {
   std::unique_ptr<llvm::MemoryBuffer> Code;
-
-  // `FileNames` must have at least "-" in it even if no file was specified.
-  assert(!FileNames.empty());
-
-  // Read in the code in case the filename alone isn't enough to detect the
-  // language.
-  ErrorOr<std::unique_ptr<MemoryBuffer>> CodeOrErr =
-      MemoryBuffer::getFileOrSTDIN(FileNames[0]);
-  if (std::error_code EC = CodeOrErr.getError()) {
-    llvm::errs() << EC.message() << "\n";
-    return 1;
+  // We can't read the code to detect the language if there's no file name.
+  if (!FileNames.empty()) {
+    // Read in the code in case the filename alone isn't enough to detect the
+    // language.
+    ErrorOr<std::unique_ptr<MemoryBuffer>> CodeOrErr =
+        MemoryBuffer::getFileOrSTDIN(FileNames[0]);
+    if (std::error_code EC = CodeOrErr.getError()) {
+      llvm::errs() << EC.message() << "\n";
+      return 1;
+    }
+    Code = std::move(CodeOrErr.get());
   }
-  Code = std::move(CodeOrErr.get());
-
   llvm::Expected<clang::format::FormatStyle> FormatStyle =
-      clang::format::getStyle(Style, IsSTDIN ? AssumeFileName : FileNames[0],
+      clang::format::getStyle(Style,
+                              FileNames.empty() || FileNames[0] == "-"
+                                  ? AssumeFileName
+                                  : FileNames[0],
                               FallbackStyle, Code ? Code->getBuffer() : "");
   if (!FormatStyle) {
     llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
@@ -682,11 +684,8 @@ int main(int argc, const char **argv) {
     return 0;
   }
 
-  if (FileNames.empty())
-    FileNames.push_back("-");
-
   if (DumpConfig)
-    return dumpConfig(FileNames[0] == "-");
+    return dumpConfig();
 
   if (!Files.empty()) {
     std::ifstream ExternalFileOfFiles{std::string(Files)};
@@ -699,7 +698,10 @@ int main(int argc, const char **argv) {
     errs() << "Clang-formating " << LineNo << " files\n";
   }
 
-  if (FileNames.size() != 1 &&
+  if (FileNames.empty())
+    return clang::format::format("-");
+
+  if (FileNames.size() > 1 &&
       (!Offsets.empty() || !Lengths.empty() || !LineRanges.empty())) {
     errs() << "error: -offset, -length and -lines can only be used for "
               "single file.\n";
@@ -709,14 +711,13 @@ int main(int argc, const char **argv) {
   unsigned FileNo = 1;
   bool Error = false;
   for (const auto &FileName : FileNames) {
-    const bool IsSTDIN = FileName == "-";
-    if (!IsSTDIN && isIgnored(FileName))
+    if (FileName != "-" && isIgnored(FileName))
       continue;
     if (Verbose) {
       errs() << "Formatting [" << FileNo++ << "/" << FileNames.size() << "] "
              << FileName << "\n";
     }
-    Error |= clang::format::format(FileName, IsSTDIN);
+    Error |= clang::format::format(FileName);
   }
   return Error ? 1 : 0;
 }

Copy link
Contributor

@bhamiltoncx bhamiltoncx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you add one more test, please?

clang/test/Format/dump-config.cpp Outdated Show resolved Hide resolved
@owenca owenca merged commit 8f6e13e into llvm:main Feb 8, 2024
4 checks passed
@owenca owenca deleted the 80621 branch February 8, 2024 05:35
@owenca owenca added this to the LLVM 18.X Release milestone Feb 8, 2024
@owenca
Copy link
Contributor Author

owenca commented Feb 8, 2024

/cherry-pick 8f6e13e

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 8, 2024
Commit d813af7 addressed a regression introduced by commit
3791b3f
but caused `clang-format -dump-config` to "hang".

This patch reverts changes to ClangFormat.cpp by both commits and
reworks the cleanup.

Fixes llvm#80621.

(cherry picked from commit 8f6e13e)
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2024

/pull-request #81096

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 9, 2024
Commit d813af7 addressed a regression introduced by commit
3791b3f
but caused `clang-format -dump-config` to "hang".

This patch reverts changes to ClangFormat.cpp by both commits and
reworks the cleanup.

Fixes llvm#80621.

(cherry picked from commit 8f6e13e)
@tstellar
Copy link
Collaborator

tstellar commented Feb 9, 2024

Tracking the backport request here: #81096

tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Commit d813af7 addressed a regression introduced by commit
3791b3f
but caused `clang-format -dump-config` to "hang".

This patch reverts changes to ClangFormat.cpp by both commits and
reworks the cleanup.

Fixes llvm#80621.

(cherry picked from commit 8f6e13e)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Commit d813af7 addressed a regression introduced by commit
3791b3f
but caused `clang-format -dump-config` to "hang".

This patch reverts changes to ClangFormat.cpp by both commits and
reworks the cleanup.

Fixes llvm#80621.

(cherry picked from commit 8f6e13e)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Commit d813af7 addressed a regression introduced by commit
3791b3f
but caused `clang-format -dump-config` to "hang".

This patch reverts changes to ClangFormat.cpp by both commits and
reworks the cleanup.

Fixes llvm#80621.

(cherry picked from commit 8f6e13e)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

clang-format -dump-config "hangs"
4 participants