Page MenuHomePhabricator

[flang][Driver] Refine _when_ driver diagnostics are formatted
ClosedPublic

Authored by peixin on May 22 2022, 4:34 AM.

Details

Summary

This patch refines when driver diagnostics are formatted so that
flang-new and flang-new -fc1 behave consistently with clang and
clang -cc1, respectively. This change only applies to driver diagnostics.
Scanning, parsing and semantic diagnostics are separate and not covered here.

NEW BEHAVIOUR
To illustrate the new behaviour, consider the following input file:

! file.f90
program m
  integer :: i = k
end

In the following invocations, "error: Semantic errors in file.f90" _will be_
formatted:

$ flang-new file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 -fcolor-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

However, in the following invocations, "error: Semantic errors in file.f90"
_will not be_ formatted:

$ flang-new -fno-color-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

Before this change, none of the above would be formatted. Note also that the
default behaviour in flang-new is different to flang-new -fc1 (this is
consistent with Clang).

NOTES ON IMPLEMENTATION
Note that the diagnostic options are parsed in createAndPopulateDiagOpts in
driver.cpp. That's where the driver's DiagnosticEngine options are set. Like
most command-line compiler driver options, these flags are "claimed" in
Flang.cpp (i.e. when creating a frontend driver invocation) by calling
getLastArg rather than in driver.cpp.

In Clang's Options.td, defm color_diagnostics is replaced with two separate
definitions: def fcolor_diagnostics and def fno_color_diagnostics`. That's
because originally color_diagnostics derived from OptInCC1FFlag, which is a
multiclass for opt-in options in CC1. In order to preserve the current
behaviour in clang -cc1 (i.e. to keep -fno-color-diagnostics unavailable in
clang -cc1) and to implement similar behaviour in flang-new -fc1, we can't
re-use OptInCC1FFlag.

Formatting is only available in consoles that support it and will normally mean that
the message is printed in bold + color.

Co-authored-by: Andrzej Warzynski <andrzej.warzynski@arm.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
awarzynski added inline comments.May 31 2022, 7:20 AM
flang/lib/Frontend/CompilerInstance.cpp
189 โ†—(On Diff #431226)

This should be controllable by -f{no}-color-diagnostics.

flang/lib/Frontend/TextDiagnosticPrinter.cpp
51 โ†—(On Diff #431226)

I'm not a fan of including semantic details in variable names. I've also not encountered it before in LLVM, have you?

diagsEng (or diagsEngine) would remain valid even if people decide to change & to *. Why not use that instead?

flang/test/Driver/driver-help-hidden.f90
75 โ†—(On Diff #431226)

In tests, we only need the error message - the actual formatting is irrelevant. In fact, it complicates testing because it forces you to add {{.*}}.

My point being - the diagnostics in tests should be displayed without any formatting. If you want the formatting to be enabled by default, then you will have to use -fno-color-diagnostics in tests (alternatively, update the %flang and %flang_fc1 LIT variables - that would probably be cleaner). I can't think of any other solution at the moment ๐Ÿค” . Clang's equivalent of -verify would be much better, but sadly that's not available.

Thanks @awarzynski for these suggestions.

Hi @peixin, thanks for sending this!

I think that formatting diagnostics is very useful, but we should avoid having the formatting "ON" at all times. We should also allow users to turn the formatting "OFF". More comments below and inline.

Enable the color set by default for diag messages.

  1. Is the new behavior consistent with Clang? In general, we should follow the principle of least surprise and make flang-new as consistent with clang as possible (within reason). And if we decide to diverge, then we should document the rationale.
  2. Does it affect the driver (flang-new & flang-new -fc1) and/or the frontend diagnostics? Note that unlike in Clang, driver and frontend diagnostics in Flang are separate. It would be good to document more specifically what is being updated here (from what I can see, just the driver).

I checked with clang, and it has the option you mentioned. I think clang enable it by default. I can see colorful diag messages on my terminal by default.

$ clang --help  | grep color
  -fcolor-diagnostics     Enable colors in diagnostics
  -fno-color-diagnostics  Disable colors in diagnostics
flang/lib/Frontend/CompilerInstance.cpp
189 โ†—(On Diff #431226)

Agree. Clang does it, too.

flang/lib/Frontend/TextDiagnosticPrinter.cpp
51 โ†—(On Diff #431226)

Good point. Will fix this.

flang/test/Driver/driver-help-hidden.f90
75 โ†—(On Diff #431226)

Agree with you. I didn't figure out how clang use -verify with expected-error{{message}} and expected-error{{warning}}. But I think it's OK to use the current test tools for flang. With the option, it is easy to silence the colorful diagnostics.

I checked with clang, and it has the option you mentioned. I think clang enable it by default. I can see colorful diag messages on my terminal by default.

$ clang --help  | grep color
  -fcolor-diagnostics     Enable colors in diagnostics
  -fno-color-diagnostics  Disable colors in diagnostics

Great! I think that it would be helpful to mention in the summary/commit message that:

  • with this change, driver diagnostics will be formatted by default,
  • this can be controlled with -f{no-}color-diagnosics,
  • the new behavior is consistent with clang.

Enable the color set by default for diag messages.

Note that it's not only about the colors - the formatting includes making the font "bold".

flang/test/Driver/driver-help-hidden.f90
75 โ†—(On Diff #431226)

-verify is a Clang option that configures Clang's DiagnosticsEngine. Although we do use it for driver diagnostics, language diagnostics in Flang (e.g. scanning, parsing and sema diagnostics) are completely separate. And -verify is most useful/powerful when it comes to language diagnostics.

But I think it's OK to use the current test tools for flang. With the option, it is easy to silence the colorful diagnostics.

+1

peixin updated this revision to Diff 434324.Jun 5 2022, 6:21 AM
peixin retitled this revision from [flang] Enable the color set for diag messages to [flang] Enable the color and bold set for diag messages by default.
peixin edited the summary of this revision. (Show Details)

Address the comments from @awarzynski .

Thanks, this is much shorter now :) So, with this revision I get no formatting at all. Do you? I suspect that DiagnosticsEngine is misconfigured. Note that in Clang, diagnostics are formatted by default in clang, but the formatting is disabled in clang -cc1. We need to make sure that that's the case here as well.

In particular, see how DiagnosticsOptions is created for clang: here. Compare this with the set-up for clang -cc1 here. I believe that the default value of DefaultDiagColor is key: here.

I might have missed something, but please make sure that:

  • flang-new and clang behave consistently
  • flang-new -fc1 and clang -cc1 also behave consistently
clang/include/clang/Driver/Options.td
1365โ€“1366

These are GCC spellings, which are aliases for the Clang spellings (-f{no-}color-diagnostics) and which are already enabled for flang-new. Is this change required at all?

clang/lib/Driver/ToolChains/Flang.cpp
59โ€“60

Would you mind moving this to a dedicated method? For example, something akin Clang's RenderDiagnosticsOptions. I know that it's only for 2 options, but there could be many more in the future and I think that it's nice to categorise them like this.

flang/lib/Frontend/CompilerInvocation.cpp
505โ€“506
peixin added a comment.Jun 6 2022, 6:15 AM

Thanks @awarzynski for the explanations. I misunderstood flang-new -fc1. Will fix this patch.

So, with this revision I get no formatting at all. Do you?

What do you mean by this? With thie revision, I got color bold for both flang-new and flang-new -fc1.

$ cat case.f90 
program m
  integer :: i = k
end
$ flang-new -fc1 case.f90 
error: Semantic errors in case.f90
./case.f90:2:18: error: Must be a constant value
    integer :: i = k
                   ^

But please notice that this revision is to set the color and bold for the "error" before "Semantic errors in case.f90", not the "error" before "Must be a constant value", which is planned in D126166.

clang/lib/Driver/ToolChains/Flang.cpp
59โ€“60

Agree. I also saw a number of diagnostic options in Options.td. Will fix this.

So, with this revision I get no formatting at all. Do you?

What do you mean by this? With thie revision, I got color bold for both flang-new and flang-new -fc1.

Ignore that comment - one of these days! ;)

peixin updated this revision to Diff 435025.Jun 7 2022, 7:14 PM
peixin retitled this revision from [flang] Enable the color and bold set for diag messages by default to [flang] Enable the color and bold set for diag messages.
peixin edited the summary of this revision. (Show Details)
peixin added a comment.Jun 7 2022, 7:18 PM

Address the comments from @awarzynski .

Please notice -fno-color-diagnostics is not supported in clang -cc1. This option is supported in flang-new -fc1 even though no-color is enabled by default for flang-new -fc1 since I don't see any reason to disallow it.

clang/include/clang/Driver/Options.td
1365โ€“1366

When I disable one of them, I cannot get the color and bold messages.

flang/lib/Frontend/CompilerInvocation.cpp
505โ€“506

Fixed.

peixin edited the summary of this revision. (Show Details)Jun 7 2022, 7:20 PM
awarzynski added a comment.EditedJun 8 2022, 4:52 AM
$ flang-new a.f90 ! enable color and bold
$ flang-new -fc1 a.f90 ! disable color and bold
$ flang-new -fcolor-diagnostics a.f90 ! enable color and bold
$ flang-new -fcolor-diagnostics -fc1 a.f90 ! enable color and bold
$ flang-new -fno-color-diagnostics a.f90 ! disable color and bold
$ flang-new -fno-color-diagnostics -fc1 a.f90 ! disable color and bold
  1. -fc1 needs to be the very first option, so flang-new -fno-color-diagnostics -fc1 doesn't make much sense
  1. enable color and bold -> where?
    • In compiler driver diagnostics?
    • In frontend driver diagnostics?
    • In frontend (i.e. sema, parsing) diagnostics?
  1. $ flang-new a.f90 ! enable color and bold -> I get no formatting with this revision.

Re 3, I believe that that's down to your changes in "TextDiagnosticPrinter.cpp". Since you are no longer using DiagnosticOptions directly, you will have to make sure that options in DiagnosticsEngine are set-up correctly. More specifically, there's ShowColors member variable in two places:

These are separate and independent member variables. Before this change, AFAIK, Flang used ShowColors from DiagnosticOptions. Your patch changes that - is this required?

To me, the most natural approach would be to follow Clang's example:

  1. flang-new should have the formatting enabled by default
  2. flang-new -fc1 should have the formatting disabled by default
  3. flang-new should forward it's settings to flang-new -fc1 when constructing a frontend driver job.

Item 3 above should translate to (skipping irrelevant frontend driver options):

  • flang-new file.f90 --> flang-new -fc1 -fcolor-diagnostics file.f90
  • flang-new -fno-color-diagnostics file.f90 --> flang-new -fc1 -fno-color-diagnostics file.f90 (or flang-new -fc1 file.f90)

There is a lot in here - I hope that this is not discouraging! Unfortunately, the diagnostics API that the driver library is using is very complex and convoluted. And additionally, we want different default behavior in every driver, which makes all this even more confusing. I'm hoping that this is helpful.

flang/include/flang/Frontend/FrontendOptions.h
222

This is out of order:

flang/include/flang/Frontend/FrontendOptions.h:217:9: error: field 'needProvenanceRangeToCharBlockMappings' will be initialized after field 'showColors' [-Werror,-Wreorder-ctor]
        needProvenanceRangeToCharBlockMappings(false), showColors(false) {}
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~~
        showColors(false)                              needProvenanceRangeToCharBlockMappings(false)
awarzynski added inline comments.Jun 8 2022, 5:22 AM
clang/include/clang/Driver/Options.td
1365โ€“1366

I would like to avoid any changes in Options.td unless ... unavoidable :)

When I disable one of them, I cannot get the color and bold messages.

Could you explain why?

peixin updated this revision to Diff 435405.Jun 8 2022, 7:07 PM
peixin retitled this revision from [flang] Enable the color and bold set for diag messages to [flang][Driver] Set the color and bold set for diag messages.
peixin edited the summary of this revision. (Show Details)
peixin added a comment.Jun 8 2022, 7:12 PM

@awarzynski Thanks a lot for the detailed explanations. I think doing two things in one patch gets me into trouble. I had thought flang-new -fc1 works as part of flang-new. Apparently, they have different philosophy. Now I make it clear. Add in the summary to describle what were missed before and what are changed in this patch and why.

clang/include/clang/Driver/Options.td
1365โ€“1366

I made something wrong for this. Now fixed.

flang/include/flang/Frontend/FrontendOptions.h
222

Thanks for the notice. Fixed.

@peixin Thanks for the updates!

After parsing the option, it cannot be added to CmdArgs in Flang.cpp since the -fc1 is added by default for the compiler driver and the option -fno-color-diagnostics conflicts with -fc1. Instead, the parsing result is stored in diags options and push -fcolor-diagnostics to CmdArgs only if the ShowColors label of diags options is true.

I don't believe that's true :) How about this:

For the compiler driver, flang-new, the diagnostic options are parsed in createAndPopulateDiagOpts in driver.cpp. That's where the driver's DiagnosticEngine options are set. Like most command-line compiler driver options, these flags are "claimed" in Flang.cpp (i.e. when creating a frontend driver invocation) by calling getLastArg rather than in driver.cpp.

clang/include/clang/Driver/Options.td
1358โ€“1363

This change makes sense to me. Basically, you are making sure that both invocations below will behave consistently:

$ clang -cc1 -fno-color-diagnostics file.c
error: unknown argument: '-fno-color-diagnostics'
$ flang-new -fc1 -fno-color-diagnostics file.f90
error: unknown argument: '-fno-color-diagnostics'

In particular, you avoid using OptInCC1FFlag for flang-new -fc1 (which, IMO, would be counterintuitive). But it would be great to double-check this with somebody from the Clang side of the universe :)

@jansvoboda11 @MaskRay Is this change in Options.td OK with you? Thanks!

clang/lib/Driver/ToolChains/Flang.cpp
112

IIRC, the diagnostic options are parsed here. So when we get here, these options should ideally be already "claimed". Could you expand the comment, please? The reason behind "argument unused" is unclear.

MaskRay added inline comments.Jun 13 2022, 11:21 AM
clang/include/clang/Driver/Options.td
1358โ€“1363

I confirm that the behavior for Clang is still desired: both -f[no-]color-diagnostics are usable for driver while only the positive option exists for cc1.

But I am puzzled by this def fcolor_diagnostics change. Why can't flang-new use its own Options.td file?

awarzynski added inline comments.Jun 13 2022, 12:04 PM
clang/include/clang/Driver/Options.td
1358โ€“1363

I confirm that the behavior for Clang is still desired

Thanks!

But I am puzzled by this def fcolor_diagnostics change. Why can't flang-new use its own Options.td file?

How would this help (i.e. without a major refactor)? IIUC, clangDriver creates one global instance of OptTable . So regardless whether there's a dedicated Options.td for flang-new or not, we'd be including the one for Clang that already defines these options, i.e.:

defm color_diagnostics : OptInCC1FFlag<"color-diagnostics", "Enable", "Disable", " colors in diagnostics",
  [CoreOption, FlangOption]>;

Separately, we'd need to define something similar for flang-new, e.g.:

defm flang_color_diagnostics : OptInFC1FFlag<"color-diagnostics", "Enable", "Disable", " colors in diagnostics",
  [CoreOption, FlangOption]>;

But how would the clangDriver parse -fcolor-diagnotiscs? As OPT_color_diagnostics or as OPT_flang_color_diagnostics?

I might be missing something obvious here. Until recently there have only been a handful of flang-new options and we've not investigated splitting Options.td too much. From my initial impression, that would require a fair bit of re-design work (e.g. shouldn't we extract a dedicated *.td file for clang -cc1 as well?).

I think that that's something outside the scope of this patch, but I would definitely support that. Is that what you had in mind?

MaskRay added inline comments.Jun 13 2022, 1:49 PM
clang/include/clang/Driver/Options.td
1358โ€“1363

My question is why do these flang frontend need the Clang Options.td. It is like: rust/swift are not supposed to use Clang Options.td. They define their own options.

Does flang intend to support most Clang options so that it reuses Clang Options.td?

peixin updated this revision to Diff 436692.Jun 14 2022, 1:21 AM
peixin edited the summary of this revision. (Show Details)

@peixin Thanks for the updates!

After parsing the option, it cannot be added to CmdArgs in Flang.cpp since the -fc1 is added by default for the compiler driver and the option -fno-color-diagnostics conflicts with -fc1. Instead, the parsing result is stored in diags options and push -fcolor-diagnostics to CmdArgs only if the ShowColors label of diags options is true.

I don't believe that's true :) How about this:

For the compiler driver, flang-new, the diagnostic options are parsed in createAndPopulateDiagOpts in driver.cpp. That's where the driver's DiagnosticEngine options are set. Like most command-line compiler driver options, these flags are "claimed" in Flang.cpp (i.e. when creating a frontend driver invocation) by calling getLastArg rather than in driver.cpp.

Thanks for this. Your desciptions are better.

Considering @awarzynski contributes a lot to this patch, add as co-author.

clang/lib/Driver/ToolChains/Flang.cpp
112

Borrow the comment from Clang.cpp.

awarzynski added inline comments.Jun 14 2022, 2:13 AM
clang/include/clang/Driver/Options.td
1358โ€“1363

My question is why do these flang frontend need the Clang Options.td. It is like: rust/swift are not supposed to use Clang Options.td. They define their own options.

Flang is an in-tree project that builds its compiler driver in terms of clangDriver (CMake rule). Neither Rust nor Swift are in-tree (or use clangDriver, AFAIK).

Does flang intend to support most Clang options so that it reuses Clang Options.td?

No. But Options.td is already being shared between clang and clang -cc1, and these drivers don't share all of their options. Also, Options.td contained various Fortran flags even before LLVM Flang was merged into LLVM. My point being - Options.td is quite a mix regardless of whether LLVM Flang is there or not.

Btw, here's Flang driver documentation, and here's the original RFC in which it was proposed for LLVM Flang to re-use Clang's clangDriver. Just in case you were interested in the wider context (perhaps you're already familiar with this).

@peixin Thanks again for improving this! All in all it makes sense to me. However, given that I am now a co-author here (thanks for that!), we should make sure that at least one more reviewer approves this change :)

One other thing: given how fiddly and nuanced this change is, I would appreciate a bit more refined summary. Would be open to updating it? How about the following:

UPDATED SUMMARY BELOW

[flang][Driver] Refine _when_ driver diagnostics are formatted

This patch refines when driver diagnostics are formatted so that
flang-new and flang-new -fc1 behave consistently with clang and
clang -cc1, respectively. This change only applies to driver diagnostics
Scanning, parsing and semantic diagnostics are separate and not covered here.

NEW BEHAVIOUR
To illustrate the new behaviour, consider the following input file:

! file.f90
program m
  integer :: i = k
end

In the following invocations, "error: Semantic errors in file.f90" _will be_
formatted:

$ flang-new file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 -fcolor-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

However, in the following invocations, "error: Semantic errors in file.f90"
_will not be_ formatted:

$ flang-new -fno-color-diagnostics file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k
$ flang-new -fc1 file.f90
error: Semantic errors in file.f90
./file.f90:2:18: error: Must be a constant value
    integer :: i = k

Before this change, none of the above would be formatted. Note also that the
default behaviour in flang-new is different to flang-new -fc1 (this is
consistent with Clang).

NOTES ON IMPLEMENTATION
Note that the diagnostic options are parsed in createAndPopulateDiagOpts in
driver.cpp. That's where the driver's DiagnosticEngine options are set. Like
most command-line compiler driver options, these flags are "claimed" in
Flang.cpp (i.e. when creating a frontend driver invocation) by calling
getLastArg rather than in driver.cpp.

In Clang's Options.td, defm color_diagnostics is replaced with two separate
definitions: def fcolor_diagnostics and def fno_color_diagnostics`. That's
because originally color_diagnostics derived from OptInCC1FFlag, which is a
multiclass for opt-in options in CC1. In order to preserve the current
behaviour in clang -cc1 (i.e. to keep -fno-color-diagnostics unavailable in
clang -cc1) and to implement similar behaviour in flang-new -fc1, we can't
re-use OptInCC1FFlag.

Formatting is only available in consoles that support it and will normally mean that
the message is printed in bold + color.

peixin retitled this revision from [flang][Driver] Set the color and bold set for diag messages to [flang][Driver] Refine _when_ driver diagnostics are formatted.Jun 17 2022, 10:29 PM
peixin edited the summary of this revision. (Show Details)

One other thing: given how fiddly and nuanced this change is, I would appreciate a bit more refined summary. Would be open to updating it? How about the following:

@awarzynski Thanks a lot for the summary. I think it's much better and more user-friendly. The patch supporting one feature of driver really should give the direction of how to use it in the commit message (summary). Updated it as suggested.

peixin added inline comments.Jun 20 2022, 4:15 AM
clang/include/clang/Driver/Options.td
1358โ€“1363

My question is why do these flang frontend need the Clang Options.td. It is like: rust/swift are not supposed to use Clang Options.td. They define their own options.

Flang is an in-tree project that builds its compiler driver in terms of clangDriver (CMake rule). Neither Rust nor Swift are in-tree (or use clangDriver, AFAIK).

Does flang intend to support most Clang options so that it reuses Clang Options.td?

No. But Options.td is already being shared between clang and clang -cc1, and these drivers don't share all of their options. Also, Options.td contained various Fortran flags even before LLVM Flang was merged into LLVM. My point being - Options.td is quite a mix regardless of whether LLVM Flang is there or not.

Btw, here's Flang driver documentation, and here's the original RFC in which it was proposed for LLVM Flang to re-use Clang's clangDriver. Just in case you were interested in the wider context (perhaps you're already familiar with this).

1358โ€“1363
rovka added a subscriber: rovka.Jun 21 2022, 12:20 AM

Hi Peixin,

I think this needs a few more tests. You should at least check that -fcolor-diagnostics is correctly forwarded to flang-new -fc1 (maybe add to this test), and also check that -fcolor-diagnostics and -fno-color-diagnostics are accepted or rejected by flang-new and flang-new -fc1. It would also be nice to check that -fcolor-diagnostics actually does something, if you can find an example where flang uses colors. You can find some inspiration in various clang or lld tests:
https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/color-diagnostics.c
https://github.com/llvm/llvm-project/blob/main/clang/test/Misc/diag-template-diffing-color.cpp
https://github.com/llvm/llvm-project/blob/main/clang/test/AST/ast-dump-color.cpp
https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/color-diagnostics.test

Hope that helps.

peixin updated this revision to Diff 438606.Jun 21 2022, 2:31 AM
peixin added a reviewer: rovka.

Add test cases as suggested. @rovka Thanks for the provided info. It really helps.

Thanks for the tests @peixin - they will be very helpful! I've left a few suggestions to make them a bit stricter. WDYT?

flang/test/Driver/color-diagnostics.f90
1

[nit] You are also testing the behavior of -f{no-}color-diagnostics here :)

2

Can you explain why? I'm guessing it's the same reason as https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/color-diagnostics.test#L1.

10โ€“11

Could you also test:

! RUN: not %flang_fc1 %s 2>&1

?

17

This is identical to COMPILER_DRIVER_DC - why not use one LIT variable for this?

flang/test/Driver/frontend-forwarding.f90
21โ€“39
  1. I'd move it to a dedicated file. In the RUN line above, everything is just unconditionally forwarded for flang-new -fc1. In here, there's quite a lot of extra logic involved that's specific to -f{no-}color-diagnosstics. Alternatively, could you add comments to explain what this new block is meant to test?
  2. What's CHECK-DC? Perhaps you meant CHECK-CD instead (i.e. CHECK-{C}OLOR_{D}IAGNOSTICS)?
  3. ! CHECK-DC: "-fcolor-diagnostics" - could you be a bit more stricter here and replace with ! CHECK-DC: "-fc1"{{.*}} "-fcolor-diagnostics" ?
peixin updated this revision to Diff 438628.Jun 21 2022, 3:54 AM

Format the test cases as @awarzynski suggested.

Thanks! LGTM :)

To me, Fangrui is OK with this change provided that it does not alter the behavior in Clang (that's indeed the case here). So, I would go ahead and merge it regardless of whether there's an explicit LGTM from a Clang developer or not. Unless there's more feedback!

Please, wait for Diana to confirm that she is also OK with the updates before merging.

Thanks for all the effort and for addressing all my comments - that's much appreciated!

rovka accepted this revision.Jun 21 2022, 6:08 AM

Yep, LGTM, thanks for the tests!
(Disclaimer: I don't consider myself a Clang developer, so maybe wait a day or 2 before committing in case someone else has comments)

This revision is now accepted and ready to land.Jun 21 2022, 6:08 AM

Yep, LGTM, thanks for the tests!
(Disclaimer: I don't consider myself a Clang developer, so maybe wait a day or 2 before committing in case someone else has comments)

Sure. Will wait for one more day before committing.

Herald added a project: Restricted Project. ยท View Herald TranscriptJun 22 2022, 8:58 AM
Herald added a subscriber: cfe-commits. ยท View Herald Transcript