-fdiagnostics-format= design review

Clang Frontend
1

Doug,

Thanks for the help on this. Please review:

**-fdiagnostics-format=none/clang/msvc/vi**: Changes diagnostic output format to better match IDEs and command line tools.
This option, which defaults to "none", controls the output format of the filename, line number, and column printed in diagnostic messages.

For example, a format string warning will produce these three renditions based on the setting of this option:

  t.c:3:11: warning: conversion specifies type 'char *' but the argument has type 'int'

  t.c(3) : warning: conversion specifies type 'char *' but the argument has type 'int'  

  t.c +3: warning: conversion specifies type 'char *' but the argument has type 'int'

Here is the patch:

I was unclear in CompilerInvocation.cpp if using Args.hasArg(OPT_fms_extensions) is correct.
Also I’m not sure if my usage of “none” is correct.

In TextDiagnosticPrinter.cpp I removed the suppression of DiagOpts->ShowColumn, and made the defaults for msvc turn off ShowColumn. As this seemed like a better way to do it?

So with this patch we have the same defaults as before, but with more control over the diagnostic format from the command line.

fdiagnostics-formant.patch (6.07 KB)

2

Update patch to use an enum, per Doug’s suggestion.

fdiagnostics-formant.patch (6.17 KB)

3

Why is there a value “none”?

Sebastian

4

Looks like I missed my last sentence:

The value none means pick a default that best matches the triple being used. Maybe default would be a better name for this?

I’m good with making the default clang, but I was trying to keep the current behavior of the defaults for a Windows ABI flavor of ABI being msvc.

Good catch,

Andrew

5

I think you intended this:

  • Opts.Format = 1;

  • Opts.ShowColumn = DiagnosticOptions::Msvc;

To be:

  • Opts.Format = DiagnosticOptions::Msvc;

  • Opts.ShowColumn = false;

At least in Visual Studio 2010 columns are supported with the following format:

t.c:(3,10)

I not entirely sure but I think you need ColNo-1 on MSVC

Also, shouldn’t there be tests for this?

Regards,

Erik

6

@@ -86,6 +90,7 @@
     ShowNames = 0;
     ShowOptionNames = 0;
     ShowCategories = 0;
+ Format = 0;
     ShowSourceRanges = 0;
     ShowParseableFixits = 0;
     VerifyDiagnostics = 0;

Please use:

  Format = Clang;

Index: lib/Frontend/CompilerInvocation.cpp

7

I think you intended this:
+ Opts.Format = 1;
+ Opts.ShowColumn = DiagnosticOptions::Msvc;

To be:
+ Opts.Format = DiagnosticOptions::Msvc;
+ Opts.ShowColumn = false;

Yes, sorry about that.

In DiagnosticOptions.h why is it

ShowColumn = 1;

and not

ShowColumn = true;

I was just trying to match existing style by using 0 and 1.

At least in Visual Studio 2010 columns are supported with the following format:
t.c:(3,10)
I not entirely sure but I think you need ColNo-1 on MSVC

I can add -fshow-column support for msvc to the patch, but I don't have a copy of Visual Studio 2010 to test with. Can you test it with Visual Studio 2010 when I add it?

Also, shouldn't there be tests for this?

Can you point me in the direction of how to write the tests, or even where the current tests are?

8

And a switch, maybe?

Sebastian

9

I think you intended this:
+ Opts.Format = 1;
+ Opts.ShowColumn = DiagnosticOptions::Msvc;

To be:
+ Opts.Format = DiagnosticOptions::Msvc;
+ Opts.ShowColumn = false;

Yes, sorry about that.

In DiagnosticOptions.h why is it

ShowColumn = 1;

and not

ShowColumn = true;

I was just trying to match existing style by using 0 and 1.

I have no idea :slight_smile:

I would have used true/false, but it doesn't really matter.

At least in Visual Studio 2010 columns are supported with the following format:
t.c:(3,10)
I not entirely sure but I think you need ColNo-1 on MSVC

I can add -fshow-column support for msvc to the patch, but I don't have a copy of Visual Studio 2010 to test with. Can you test it with Visual Studio 2010 when I add it?

That'd be great; someone with Visual Studio 2010 can confirm the format or submit a patch to tweak it if needed.

Also, shouldn't there be tests for this?

Can you point me in the direction of how to write the tests, or even where the current tests are?

The tests for diagnostic formatting are in test/Misc. A good example there is test/Misc/caret-diags-macros.c, which uses FileCheck to verify the output:

  http://llvm.org/cmds/FileCheck.html

  - Doug