OpWriter.ColoringOption proposal

[asotona]

I propose OpWriter enables simple ANSI coloring based on jdk.incubator.code.extern.OpWriter.COLOR system property.

ANSI coloring sample:

---

Progress

• Change must not contain extraneous whitespace

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/546/head:pull/546
$ git checkout pull/546

Update a local copy of the PR:
$ git checkout pull/546
$ git pull https://git.openjdk.org/babylon.git pull/546/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 546

View PR using the GUI difftool:
$ git pr show -t 546

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/546.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back asotona! A progress list of the required criteria for merging this PR into code-reflection will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@asotona This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

OpWriter.ColoringOption proposal

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 10 new commits pushed to the code-reflection branch:

• 32003dd: Fixes for cuda codegen issues
• 4cd871e: Add buffer tagging and opt-out moduleOp backend
• 4330ec2: HAT more consistent use of scoped code builder context + removed initialization of KernelContext in kernels
• ... and 7 more: https://git.openjdk.org/babylon/compare/dbadf0d0d7e74227bc9e55055f651001140f5ca9...code-reflection

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the code-reflection branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 03: Full - Incremental (30a37060)
• 02: Full - Incremental (d4501a1f)
• 01: Full - Incremental (095a11c7)
• 00: Full (0d3dadca)

[grfrost]

I like the idea of making OpWriter more flexible and feel this will be useful tooling. I have suggested changes in this API for a while ;)

I personally think this solution is too concrete, and we should offer a 'colorize' iface (BiFunction(Sttring,Tag, String) which we can pass to OpWriter.toText().

The default implementation of this iBiFunction would just return the String passed to it.

OpWriter would then call this colorizer with a 'tag' enum/type (identifier, const, attribute, SSAid) and a String to be colorised, and the Colorizer/BiFunction gets to return a suitable colorised version ANSI/CSI/HTML/.... whatever which gets written to the PrintWriter.

[asotona]

Yes, I was also thinking to expose the internal BiFunction<Class<? extends CodeItem>, String, String>.
However then I realized that the colorings should be concrete and simple to enable.
Maybe even affect the default op.toText() by a system property (similar to Maven -Dstyle.color=(auto|always|never)).

[asotona]

Based on the offline discussion I would like to split this effort.
This PR should provide just basic ANSI coloring triggered by a System property.
A more advanced tool exporting code model into various colored formats may be a subject of another PR and probably not directly into the code reflection module.

[PaulSandoz]

Internal property is good, avoiding any API changes.

[grfrost]

As I mentioned in slack. My problem here that that , if we have code which generates text from OpWriter buried in a lib call that you are unaware of, and you add this property on the command line (because you want to colorize some output from your call) everyone now needs to defecnd their code against these CSI/ANSI sequences.

Specifically HAT was forced to reparse the output from OpWriter to apply its formmatting/tooling, this code is definately not expecting to see CSI/ANSI escape sequences in the output from OpWriter.

I am in general favor of a solution, but the caller of OpWriter needs direct control of formatting, it should not be based on a global property IMHO.

[asotona]

I see the problem. OpWriter declares:
"A code model in textual form may be parsed back into the runtime form by parsing it."

Technically it should be easy for OpParser to ignore the coloring.
The question here is if a 3rd-party parsers should be able to parse it (effectively making the textual form an API)?

[PaulSandoz]

It's really hard to get agreement in this area of writing and reading (the larger elephant in the room is the lack of specification for the grammar, there are higher priority areas to focus on). At this point i think it best we do nothing in the incubator module and instead focus on external tooling, like in hat but perhaps moved out to a separate project.

[PaulSandoz]

Cute hack, but it makes me uncomfortable.