[PATCH] D93528: [clang-format] Add basic support for formatting JSON

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.



> With my maintainer-of-`Support/JSON` hat on, I don't like the idea of these features being shoehorned into the library to make clang-format an incrementally more featureful JSON formatter. They're likely to lead to a lot of conceptual+implementation+API complexity in a library that solves its primary use cases quite well and simply.

No plans at all to touch Support/JSON, if this needed a separate reformat() for json then I'd say I'm more likely to do that in lib/Format as we'd only need a "PrettyPrint" type function which supported the options we want to support

> So if the plan is to
>
> - use this implementation to bootstrap JSON-in-clang-format

That was kind of the plan

      

> - with a firm intention of replacing it with parsing/formatting logic that lives inside clang-format itself

So I tried again to do this yesterday, but once again seems to fail, mainly around the area of clang-formatting trying to reflow and rejoin split lines but also the level of indentation is off.

I'm starting to wonder if perhaps it needs its own separate pass, use more of clang-format but less of the actual "parsing into known structures"...

> - long-term, maybe making use of `Support/JSON` long-term for some low level bits (normalizing string tokens?) but not where flexibility is needed

I think I'd really just like to use the `FormatTokens`, but I agree if we ended up writing a specific JSON pretty-printer I could see using the inner parts (but I really have no interesting in touching lib/Support/JSON at all)

> then I think this is great. However I'm a bit leery if the plan is "land this a MVP, and then play it by ear".

Sorry I'm not sure I understand what MVP means in this context?



================
Comment at: clang/unittests/Format/FormatTestJson.cpp:14
+
+#define DEBUG_TYPE "format-test"
+
----------------
HazardyKnusperkeks wrote:
> I don't know what's the practice right now. But I would suggest renaming that to "format-test-json", so you can see directly that's for JSON.
I'm not sure of the convention or even where/when its used (I just know you need it)

lets take that change offline, we should either change them all or none of the them.

```
FormatTest.cpp:#define DEBUG_TYPE "format-test"
FormatTestCSharp.cpp:#define DEBUG_TYPE "format-test"
FormatTestComments.cpp:#define DEBUG_TYPE "format-test"
FormatTestJS.cpp:#define DEBUG_TYPE "format-test"
FormatTestJava.cpp:#define DEBUG_TYPE "format-test"
FormatTestJson.cpp:#define DEBUG_TYPE "format-test"
FormatTestObjC.cpp:#define DEBUG_TYPE "format-test"
FormatTestProto.cpp:#define DEBUG_TYPE "format-test"
FormatTestRawStrings.cpp:#define DEBUG_TYPE "format-test"
FormatTestSelective.cpp:#define DEBUG_TYPE "format-test"
FormatTestTableGen.cpp:#define DEBUG_TYPE "format-test"
FormatTestTextProto.cpp:#define DEBUG_TYPE "format-test"
NamespaceEndCommentsFixerTest.cpp:#define DEBUG_TYPE "namespace-end-comments-fixer-test"
SortImportsTestJS.cpp:#define DEBUG_TYPE "format-test"
SortImportsTestJava.cpp:#define DEBUG_TYPE "format-test"
SortIncludesTest.cpp:#define DEBUG_TYPE "format-test"
UsingDeclarationsSorterTest.cpp:#define DEBUG_TYPE "using-declarations-sorter-test"
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93528/new/

https://reviews.llvm.org/D93528