This is an archive of the discontinued LLVM Phabricator instance.

Make ScopedPrinter interface virtual
ClosedPublic

Authored by Jaysonyan on Nov 18 2021, 10:05 PM.

Details

Summary

Make the interface provided by ScopedPrinter virtual to support a future JSONScopedPrinter which will supply it's own implementation.

This change is split off from D114052.

Diff Detail

Event Timeline

Jaysonyan created this revision.Nov 18 2021, 10:05 PM
Jaysonyan requested review of this revision.Nov 18 2021, 10:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2021, 10:05 PM
jhenderson added inline comments.Nov 19 2021, 12:37 AM
llvm/include/llvm/Support/ScopedPrinter.h
232–234

No braces for single-line ifs.

395

You could avoid the logic duplication of arrayBegin with objectBegin (and same for the other new functions), with helper functions, private to the ScopedPrinter class, e.g:

virtual void objectBegin() {
  scopedBegin('{');
}

virtual void arrayBegin() {
  scopedBegin('[');
}

void scopedBegin(char Symbol) {
  unindent();
  startLine() << Symbol << "\n";
}

// Same again for scopedBegin(StringRef Label, char Symbol) and scopedEnd().
401

This ones a bit different, and can probably be left as printVersionInternal, if you want, as it's not part of the virtual interface (I also have no objections about the new name though).

405–467

Up to you, but I'd probably fix the case of the variables in this patch.

477–479
jhenderson added inline comments.Nov 19 2021, 12:41 AM
llvm/include/llvm/Support/ScopedPrinter.h
97

And check clang-format's output here too.

Jaysonyan updated this revision to Diff 388617.Nov 19 2021, 1:42 PM

Add abstraction to object/array begin/end.
Misc formatting and function renaming.

Jaysonyan marked 6 inline comments as done.Nov 19 2021, 1:43 PM
jhenderson accepted this revision.Nov 22 2021, 1:12 AM

LGTM, but don't submit unless the other patches in the stack are approved too.

This revision is now accepted and ready to land.Nov 22 2021, 1:12 AM

Make printFlags and printObject virtual.

Jaysonyan updated this revision to Diff 390250.Nov 28 2021, 9:46 PM

Misc formatting.

jhenderson added inline comments.Nov 29 2021, 1:09 AM
llvm/include/llvm/Support/ScopedPrinter.h
164–165

I've just remembered that it's common practice to leave the explicit llvm:: prefix on cases where the function is one of the standard algorithm-like functions, so it's probably best to keep it here.

482

I'm not a fan of this function name. In some ways, it's less raw than the regular printFlags code, as it includes names. Any particular need for it to be a different name, or could we just make use of function overloading?

jhenderson added inline comments.Nov 29 2021, 2:39 AM
llvm/include/llvm/Support/ScopedPrinter.h
318–324

FWIW, these two functions don't need to be in the interface, since they should implicitly convert to StringRef quite happily. Perhaps a separate patch to remove them?

326–329

On the note of separate patches, I'd be inclined to move this to be with the other printNumber overloads.

Jaysonyan updated this revision to Diff 390452.Nov 29 2021, 1:45 PM
Jaysonyan marked an inline comment as done.

Updated printRawFlagsImpl to be overloaded printFlagsImpl method.
Updated formatting.

Jaysonyan marked an inline comment as done.Nov 29 2021, 1:46 PM
Jaysonyan added inline comments.
llvm/include/llvm/Support/ScopedPrinter.h
482

I've updated printRawFlagsImpl to share the printFlagsImpl name and take a list of HexNumbers instead of FlagEntry.

jhenderson accepted this revision.Nov 30 2021, 1:08 AM

LGTM again.

Jaysonyan updated this revision to Diff 391203.Dec 1 2021, 9:46 PM
Jaysonyan marked an inline comment as done.

Add printList overloads (to handle future JSONScopedPrinter number and boolean printing).

Jaysonyan updated this revision to Diff 391216.Dec 1 2021, 10:42 PM

Update printList for uint8_t and int8_t and remove ArrayRef from printListImpl(...)

jhenderson added inline comments.Dec 2 2021, 12:56 AM
llvm/include/llvm/Support/ScopedPrinter.h
258

I don't think you need the explicit unsigned conversion, right?

277

Ditto (for the int conversion).

Jaysonyan updated this revision to Diff 391362.Dec 2 2021, 9:40 AM

Remove unneeded explicit cast in printList(...).

Jaysonyan marked 4 inline comments as done.Dec 2 2021, 9:42 AM
Jaysonyan added inline comments.
llvm/include/llvm/Support/ScopedPrinter.h
258

You're right, removed the conversion. Thanks for the catch.

jhenderson accepted this revision.Dec 3 2021, 12:35 AM

LGTM again!

jhenderson added inline comments.Dec 3 2021, 1:30 AM
llvm/include/llvm/Support/ScopedPrinter.h
213–215

Do we need an equivalent printList virtual method for APSInt?

Jaysonyan updated this revision to Diff 391730.Dec 3 2021, 1:51 PM
Jaysonyan marked an inline comment as done.

Add printList implementation for APSInt.

Jaysonyan marked an inline comment as done.Dec 3 2021, 1:51 PM
jhenderson accepted this revision.Dec 6 2021, 12:18 AM

LGTM again!

This revision was automatically updated to reflect the committed changes.