This is an archive of the discontinued LLVM Phabricator instance.

[Support] Replace 'DisableColors' boolean with 'ColorMode' enum
ClosedPublic

Authored by JDevlieghere on Jun 2 2020, 11:12 PM.

Details

Summary

Replace the DisableColors with a ColorMode which can be set to Auto, Enabled and Disabled. The purpose of this change is to make it possible to ignore raw_ostream::has_colors() not only for disabling colors, but also for enabling them.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jun 2 2020, 11:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 11:12 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Seems reasonable. Should there be some kind of unit-testing involved?

It might be nice (not now but in the future) to change the interface of the WithColor constructors, to change the DisableColors boolean into the ColorMode enum too.

Seems reasonable. Should there be some kind of unit-testing involved?

It is a good idea. WithColor does not have unit tests yet (llvm/unittests/Support/WithColorTest.cpp)

It might be nice (not now but in the future) to change the interface of the WithColor constructors, to change the DisableColors boolean into the ColorMode enum too.

Add unit test

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.
llvm/unittests/Support/WithColorTest.cpp
39 ↗(On Diff #268257)

Not sure if this works on Windows, I'll have to double check.

MaskRay accepted this revision.Jun 3 2020, 10:14 PM

LGTM. Hope harbomaster will tell whether this works on Windows.

This revision is now accepted and ready to land.Jun 3 2020, 10:14 PM
jhenderson accepted this revision.Jun 4 2020, 2:05 AM

LGTM, assuming the unit test works.

llvm/unittests/Support/WithColorTest.cpp
39 ↗(On Diff #268257)

I tried checking, but had trouble applying the parent patch for some reason. Not sure why, sorry.

JDevlieghere added a comment.EditedJun 5 2020, 9:43 AM

With the new approach this change isn't strictly necessary for D81058 to work, but I still think it's an (small) improvement over the status quo. I'm happy to either land or abandon the patch, whatever the community feels is most desirable.

jdenny added inline comments.Jun 5 2020, 9:50 AM
llvm/include/llvm/Support/WithColor.h
52

Please update the comments in this file.

With the new approach this change isn't strictly necessary for D81058 to work, but I still think it's an (small) improvement over the status quo. I'm happy to either land or abandon the patch, whatever the community feels is most desirable.

I'm always a fan of replacing bool arguments with a useful enum, so I'm happy if you think there's still some use in the patch.

This revision was automatically updated to reflect the committed changes.