Page MenuHomePhabricator
Differential D87774 Diff 296205 flang/lib/Frontend/TextDiagnosticPrinter.cpp

Changeset View

Standalone View

View Options

flang/lib/Frontend/TextDiagnosticPrinter.cpp

  • This file was added.
//===--- TextDiagnosticPrinter.cpp - Diagnostic Printer -------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//
//
// This diagnostic client prints out their diagnostic messages.
//
//===----------------------------------------------------------------------===//
#include "flang/Frontend/TextDiagnosticPrinter.h"
#include "flang/Frontend/TextDiagnostic.h"
#include "clang/Basic/DiagnosticOptions.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/raw_ostream.h"
using namespace Fortran::frontend;
TextDiagnosticPrinter::TextDiagnosticPrinter(
raw_ostream &os, clang::DiagnosticOptions *diags)
: os_(os), diagOpts_(diags) {}
sameeranjoshiUnsubmitted
Done
ReplyInline Actions

A silly question from what I see usually in Flang coding style.
Why isn't it defined in header file?

sameeranjoshi: A silly question from what I see usually in Flang coding style. Why isn't it defined in header…
awarzynskiAuthorUnsubmitted
Done
ReplyInline Actions

No such thing as silly questions! :)

AFAIK, there are no rules in the coding guidelines with regard to

  • where things should be defined, and
  • where things should be declared.

I use this rather generic rule of thumb: declare in headers, define in source files. In this particular case - I followed what was already in Clang. It made sense to me.

Do you think that it would better to define this in a header file?

awarzynski: No such thing as silly questions! :) AFAIK, there are no rules in the coding guidelines with…
kiranchandramohanUnsubmitted
Done
ReplyInline Actions

I think in flang the style is to declare the class in the source file if it is only used in that file. If it is used elsewhere then put it in the header. If it is used in another directory then move the header to include directory.

kiranchandramohan: I think in flang the style is to declare the class in the source file if it is only used in…
awarzynskiAuthorUnsubmitted
Done
ReplyInline Actions

This class is used in multiple files:

  • flang/lib/Frontend/CompilerInstance.{cpp|h}
  • flang/tools/flang-driver/driver.cpp
  • flang/unittests/Frontend/CompilerInstanceTest.cpp

IIUC, this is now resolved.

awarzynski: This class is used in multiple files: * flang/lib/Frontend/CompilerInstance.{cpp|h} *…
TextDiagnosticPrinter::~TextDiagnosticPrinter() {}
void TextDiagnosticPrinter::HandleDiagnostic(
clang::DiagnosticsEngine::Level level, const clang::Diagnostic &info) {
// Default implementation (Warnings/errors count).
DiagnosticConsumer::HandleDiagnostic(level, info);
// Render the diagnostic message into a temporary buffer eagerly. We'll use
// this later as we print out the diagnostic to the terminal.
llvm::SmallString<100> outStr;
info.FormatDiagnostic(outStr);
llvm::raw_svector_ostream DiagMessageStream(outStr);
kiranchandramohanUnsubmitted
Not Done
ReplyInline Actions

100? Will this contain path names by any chance?

kiranchandramohan: 100? Will this contain path names by any chance?
awarzynskiAuthorUnsubmitted
Done
ReplyInline Actions

It shouldn't. Currently these diagnostics are only used to report errors from the command line, so no paths are involved.

In Clang, similar class is used both for the driver diagnostics (i..e command line errors - no source location) and frontend diagnostics (source code errors - with source locations). However, AFAIK, this string is not used for the source location (e.g. path).

If we do use this class for more Flang diagnostics, we shouldn't use outStr for paths. This is just the message.

awarzynski: It shouldn't. Currently these diagnostics are only used to report errors from the command line…
sameeranjoshiUnsubmitted
Not Done
ReplyInline Actions

Can we use at places where LLVM data structures are used explicit llvm:: so an unknown user can easily identify where they come from?

sameeranjoshi: Can we use at places where LLVM data structures are used explicit `llvm::` so an unknown user…
awarzynskiAuthorUnsubmitted
Done
ReplyInline Actions

If we do that, the code is likely to be bloated with llvm::, which IMO could be detrimental to readability in the long term.

In Clang you will find a header file with all the forward declarations to avoid this: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/LLVM.h.

I definitely want to make reading this code easier to users, but I also think that instead of llvm:: one could use code-navigation tools (e.g. clangd). And forward declarations are fairly standard AFAIK.

However, if you still think that llvm:: would be better, I'm happy to update :)

awarzynski: If we do that, the code is likely to be bloated with `llvm::`, which IMO could be detrimental…
sameeranjoshiUnsubmitted
Done
ReplyInline Actions

I think it's better to follow what FIR(fir-dev) and llvm-project/flang does.
They generally try to fully-qualify name of library and avoid using forward declarations.

sameeranjoshi: I think it's better to follow what `FIR`(fir-dev) and `llvm-project/flang` does. They generally…
if (!prefix_.empty())
os_ << prefix_ << ": ";
// We only emit diagnostics in contexts that lack valid source locations.
kiranchandramohanUnsubmitted
Not Done
ReplyInline Actions

Is there an assumption that the prefix will not be empty?

kiranchandramohan: Is there an assumption that the prefix will not be empty?
awarzynskiAuthorUnsubmitted
Done
ReplyInline Actions

The prefix is optional. With prefix:

$ bin/flang-new
flang-new: error: no input files

Without prefix:

$ bin/flang-new
error: no input files

Both are valid.

awarzynski: The prefix is optional. With prefix: ``` $ bin/flang-new flang-new: error: no input files ```…
assert(!info.getLocation().isValid() &&
"Diagnostics with valid source location are not supported");
Fortran::frontend::TextDiagnostic::PrintDiagnosticLevel(
os_, level, diagOpts_->ShowColors);
Fortran::frontend::TextDiagnostic::PrintDiagnosticMessage(os_,
/*IsSupplemental=*/level == clang::DiagnosticsEngine::Note,
DiagMessageStream.str(), diagOpts_->ShowColors);
os_.flush();
CarolineConcattoUnsubmitted
Not Done
ReplyInline Actions

Why when it is clang::DiagnosticsEngine::Note it is Supplemental?

CarolineConcatto: Why when it is clang::DiagnosticsEngine::Note it is Supplemental?
awarzynskiAuthorUnsubmitted
Done
ReplyInline Actions

I'm not aware of any documentation for this, so here's my understanding :)

A clang::DiagnosticsEngine::Note is an additional piece of information for the user. It's neither a warning nor an error. Hence it's something _supplemental_ (i.e. additional info that could otherwise be skipped) and should not be pretty-printed. And that's the purpose of this argument.

awarzynski: I'm not aware of any documentation for this, so here's my understanding :) A `clang…
return;
}