Make ScopedPrinter interface virtual

[Jaysonyan]

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.

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

240–242

No braces for single-line ifs.

407

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().

413

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).

417–419

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

429–431

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

105

And check clang-format's output here too.

[Jaysonyan]

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

[jhenderson]

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

[Jaysonyan]

Make printFlags and printObject virtual.

[Jaysonyan]

Misc formatting.

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

172–173

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.

434

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]

llvm/include/llvm/Support/ScopedPrinter.h

330–336

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?

338–341

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

[Jaysonyan]

Updated printRawFlagsImpl to be overloaded printFlagsImpl method.
Updated formatting.

[Jaysonyan]

llvm/include/llvm/Support/ScopedPrinter.h

434

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

[jhenderson]

LGTM again.

[Jaysonyan]

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

[Jaysonyan]

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

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

266

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

285

Ditto (for the int conversion).

[Jaysonyan]

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

[Jaysonyan]

llvm/include/llvm/Support/ScopedPrinter.h

266

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

[jhenderson]

LGTM again!

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

221–223

Do we need an equivalent printList virtual method for APSInt?

[Jaysonyan]

Add printList implementation for APSInt.

[jhenderson]

LGTM again!