[clang] Add an option for hiding line numbers in diagnostics

[teemperor]

Summary

Clang offers a -f[no]-show-column flag for hiding the column numbers when printing diagnostics
but there is no option for doing the same with line numbers.

In LLDB having this option would be useful, as LLDB sometimes only knows the file name for a
SourceLocation and just assigns it the dummy line/column 1:1. These fake line/column numbers
are confusing to the user and LLDB should be able to tell clang to hide *both* the column and the
line number when rendering text diagnostics.

This patch adds a flag for also hiding the line numbers. It's not exposed via the command line flags
as it's most likely not very useful for any user and can lead to ambiguous output when the
user decides to only hide either the line or the column number (where file:1: ... could now refer
to both line 1 or column 1 depending on the compiler flags). LLDB can just access the DiagnosticOptions
directly when constructing its internal Clang instance.

The effect doesn't apply to Vi/MSVC style diagnostics because it's not defined how these diagnostic styles
would show an omitted line number (MSVC doesn't have such an option and Vi's line mode is theory
only supporting line numbers if I understand it correctly).

[thakis]

Generally seems fine, nits below:

clang/docs/UsersManual.rst

172 ↗

(On Diff #275028)

Can you say that this is enabled by default, and maybe make the example about fno- then? :)

clang/lib/Frontend/TextDiagnostic.cpp

832

put break on its own line, as-is the indentation is pretty misleading here :)

[teemperor]

• Fixed formatting in switch-case
• Fixed formatting in DiagnosticOptions.def

[teemperor]

clang/docs/UsersManual.rst

172 ↗

(On Diff #275028)

The text actually mentions the option is on by default (This option, which defaults to on, ) and there is an example for both enabled and disabled (the disabled one is in the last line). FWIW, this is actually just 1-1 copying the style of all the diagnostic options below, so if we want to make this more readable we probably should do the same for all other descriptions. I actually think that's a good idea as the default value is really hidden within the description.

I can make a followup review that fixes this for all the help text in the manual.

clang/lib/Frontend/TextDiagnostic.cpp

832

Thanks! seems I forgot to clang-format.

[thakis]

clang/test/Misc/diag-format.c

43 ↗

(On Diff #289887)

on 2nd thought: Hm, should we allow -fno-line -fshow-column? It makes the output ambiguous, and it doesn't seem very useful. Maybe fno-show-line should imply fno-show-column, and explicit fno-show-line fshow-column should produce an error?

[MaskRay]

Isn't DiagOpts->ShowLine sufficient? I don't think clang command line users will specify -fno-show-line...

[teemperor]

In D83038#2256974, @MaskRay wrote:

Isn't DiagOpts->ShowLine sufficient? I don't think clang command line users will specify -fno-show-line...

Yes, it's only a flag because that's how the other DiagOpts are tested. I don't think there is a lot of benefit for having the flag beside being consistent with the other options. If that simplifies things, I can also do test this with a unit test and remove all the command line changes?

[MaskRay]

In D83038#2257143, @teemperor wrote:

In D83038#2256974, @MaskRay wrote:

Isn't DiagOpts->ShowLine sufficient? I don't think clang command line users will specify -fno-show-line...

Yes, it's only a flag because that's how the other DiagOpts are tested. I don't think there is a lot of benefit for having the flag beside being consistent with the other options. If that simplifies things, I can also do test this with a unit test and remove all the command line changes?

Yeah, sounds good if that can simplify things.

[teemperor]

• Removed driver arg and now tested via a unit test.

[MaskRay]

clang/unittests/Frontend/TextDiagnosticTest.cpp

33

The argument comments should use a style (with a equal sign) which can be detected by https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html

39

; -> .

[teemperor]

• Fix typo and argument comments (thanks MaskRay !)

[teemperor]

@MaskRay is this ready to land? (This is technically in the 'accepted' state so this review probably disappeared from everyone's review queue, hence my ping)

[MaskRay]

clang/unittests/Frontend/TextDiagnosticTest.cpp

52

The argument comments should use a style (with a equal sign) which can be detected by https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html

[teemperor]

• Also fix the remaining argument comments.

clang/include/clang/Basic/DiagnosticOptions.def

 #  define SEMANTIC_ENUM_DIAGOPT(Name, Type, Bits, Default) \
      ENUM_DIAGOPT(Name, Type, Bits, Default)
 #endif
 
 SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0)   /// -w
 DIAGOPT(NoRewriteMacros, 1, 0)  /// -Wno-rewrite-macros
 DIAGOPT(Pedantic, 1, 0)         /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
+DIAGOPT(ShowLine, 1, 1)         /// Show line number on diagnostics.
 DIAGOPT(ShowColumn, 1, 1)       /// Show column number on diagnostics.
 DIAGOPT(ShowLocation, 1, 1)     /// Show source location information.
 DIAGOPT(ShowLevel, 1, 1)        /// Show diagnostic level.
 DIAGOPT(AbsolutePath, 1, 0)     /// Use absolute paths.
 DIAGOPT(ShowCarets, 1, 1)       /// Show carets in diagnostics.
 DIAGOPT(ShowFixits, 1, 1)       /// Show fixit information.
 DIAGOPT(ShowSourceRanges, 1, 0) /// Show source ranges in numeric form.
 DIAGOPT(ShowParseableFixits, 1, 0) /// Show machine parseable fix-its.

clang/lib/Frontend/TextDiagnostic.cpp

void TextDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
   if (!DiagOpts->ShowLocation)
     return;
 
   if (DiagOpts->ShowColors)
     OS.changeColor(savedColor, true);
 
   emitFilename(PLoc.getFilename(), Loc.getManager());
   switch (DiagOpts->getFormat()) {
-  case DiagnosticOptions::Clang: OS << ':'  << LineNo; break;
+  case DiagnosticOptions::Clang:
+    if (DiagOpts->ShowLine)
+      OS << ':' << LineNo;
+    break;
   case DiagnosticOptions::MSVC:  OS << '('  << LineNo; break;
   case DiagnosticOptions::Vi:    OS << " +" << LineNo; break;
   }
 
   if (DiagOpts->ShowColumn)
     // Compute the column number.
     if (unsigned ColNo = PLoc.getColumn()) {
       if (DiagOpts->getFormat() == DiagnosticOptions::MSVC) {

clang/unittests/Frontend/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FrontendTests
   ASTUnitTest.cpp
   CompilerInvocationTest.cpp
   CompilerInstanceTest.cpp
   FixedPointString.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
   ParsedSourceLocationTest.cpp
   PCHPreambleTest.cpp
   OutputStreamTest.cpp
+  TextDiagnosticTest.cpp
   )
 clang_target_link_libraries(FrontendTests
   PRIVATE
   clangAST
   clangBasic
   clangFrontend
   clangLex
   clangSema
   clangCodeGen
   clangFrontendTool
   clangSerialization
   )

clang/unittests/Frontend/TextDiagnosticTest.cpp

+//===- unittests/Frontend/TextDiagnosticTest.cpp - ------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Frontend/TextDiagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+/// Prints a diagnostic with the given DiagnosticOptions and the given
+/// SourceLocation and returns the printed diagnostic text.
+static std::string PrintDiag(const DiagnosticOptions &Opts, FullSourceLoc Loc) {
+  std::string Out;
+  llvm::raw_string_ostream OS(Out);
+  clang::LangOptions LangOpts;
+  // Owned by TextDiagnostic.
+  DiagnosticOptions *DiagOpts = new DiagnosticOptions(Opts);
+  TextDiagnostic Diag(OS, LangOpts, DiagOpts);
+  // Emit a dummy diagnostic that is just 'message'.
+  Diag.emitDiagnostic(Loc, DiagnosticsEngine::Level::Warning, "message",
+                      /*Ranges=*/{}, /*FixItHints=*/{});
+  OS.flush();
+  return Out;
+}
+
+TEST(TextDiagnostic, ShowLine) {
+  // Create dummy FileManager and SourceManager.
+  FileSystemOptions FSOpts;
+  FileManager FileMgr(FSOpts);
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs);
+  DiagnosticsEngine DiagEngine(DiagID, new DiagnosticOptions,
+                               new IgnoringDiagConsumer());
+  SourceManager SrcMgr(DiagEngine, FileMgr);
+
+  // Create a dummy file with some contents to produce a test SourceLocation.
+  const llvm::StringRef file_path = "main.cpp";
+  const llvm::StringRef main_file_contents = "some\nsource\ncode\n";
+  const clang::FileEntry &fe = *FileMgr.getVirtualFile(
+      file_path,
+      /*Size=*/static_cast<off_t>(main_file_contents.size()),
+      /*ModificationTime=*/0);
+
+  llvm::SmallVector<char, 64> buffer;
+  buffer.append(main_file_contents.begin(), main_file_contents.end());
+  auto file_contents = std::make_unique<llvm::SmallVectorMemoryBuffer>(
+      std::move(buffer), file_path);
+  SrcMgr.overrideFileContents(&fe, std::move(file_contents));
+
+  // Create the actual file id and use it as the main file.
+  clang::FileID fid =
+      SrcMgr.createFileID(&fe, SourceLocation(), clang::SrcMgr::C_User);
+  SrcMgr.setMainFileID(fid);
+
+  // Create the source location for the test diagnostic.
+  FullSourceLoc Loc(SrcMgr.translateLineCol(fid, /*Line=*/1, /*Col=*/2),
+                    SrcMgr);
+
+  DiagnosticOptions DiagOpts;
+  DiagOpts.ShowLine = true;
+  DiagOpts.ShowColumn = true;
+  // Hide printing the source line/caret to make the diagnostic shorter and it's
+  // not relevant for this test.
+  DiagOpts.ShowCarets = false;
+  EXPECT_EQ("main.cpp:1:2: warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  // Check that ShowLine doesn't influence the Vi/MSVC diagnostic formats as its
+  // a Clang-specific diagnostic option.
+  DiagOpts.setFormat(TextDiagnosticFormat::Vi);
+  DiagOpts.ShowLine = false;
+  EXPECT_EQ("main.cpp +1:2: warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  DiagOpts.setFormat(TextDiagnosticFormat::MSVC);
+  DiagOpts.ShowLine = false;
+  EXPECT_EQ("main.cpp(1,2): warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  // Reset back to the Clang format.
+  DiagOpts.setFormat(TextDiagnosticFormat::Clang);
+
+  // Hide line number but show column.
+  DiagOpts.ShowLine = false;
+  EXPECT_EQ("main.cpp:2: warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  // Show line number but hide column.
+  DiagOpts.ShowLine = true;
+  DiagOpts.ShowColumn = false;
+  EXPECT_EQ("main.cpp:1: warning: message\n", PrintDiag(DiagOpts, Loc));
+}
+
+} // anonymous namespace