style(clang-tidy): Clean include headers of src/core/jsonschema

[bavulapati]

Reported by clang-tidy check misc-include-cleaner
Refs: sourcemeta/blaze#429

[bavulapati]

These changes are getting bigger when I consider one folder at a time

[bavulapati]

@jviotti PTAL

[bavulapati]

@jviotti addressed it. PTAL
I'll send a separate PR for all the other modules

[bavulapati]

@jviotti Consider a public header like json.h, it includes a bunch of private headers like json_error.h, json_value.h.

What do you think about making json.h complete, having all the declarations of it's private headers?
In modules, there is a concept of re-export. If a module B is imported into module A, module A can re export the imported module A.
That would make the consumer(dev) to just have the knowledge of a single header, instead of going through chain of modules?

[bavulapati]

In our case of clang-tidy, even though the public header json.h is included, the check misc-include-cleaner expects json_error.h to be included, if using a declaration from json-error.h

[jviotti]

What do you think about making json.h complete, having all the declarations of it's private headers?
In modules, there is a concept of re-export. If a module B is imported into module A, module A can re export the imported module A.

The idea of these private headers was to make things a little bit more maintainable. Otherwise these modules often expose a lot of stuff, and the single header file would get huge

[jviotti]

These tests look off. The tests should not be including jsonschema_error if they already include jsonschema, as that one includes the other?

[bavulapati]

As I mentioned earlier, clang-tidy expects the includes to be direct not transitive

[jviotti]

This one looks off too?

[jviotti]

These ones too, as jsonschema.h already includes jsonschema_walker.h

[jviotti]

In our case of clang-tidy, even though the public header json.h is included, the check misc-include-cleaner expects json_error.h to be included, if using a declaration from json-error.h

Hmm, well that will be tricky then. This whole part of ClangTidy looks complicated. Let me do some more digging. Maybe we should start with other warnings first?

[jviotti]

Or maybe we can turn of MissingIncludes: https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html, as that's the one that forces the direct includes?

A boolean that controls whether the check should report missing includes (header files from which symbols are used but which are not directly included).

[jviotti]

See #1685. I think that is enough for now. Maybe worth trying to re-create that PR given the ClangTidy warnings with that new config?

I'll think more about a better solution long term. Maybe C++ modules fixes all of this.

[bavulapati]

Hmm, well that will be tricky then. This whole part of ClangTidy looks complicated. Let me do some more digging. Maybe we should start with other warnings first?

Yes, we can try that too.

[bavulapati]

See #1685. I think that is enough for now. Maybe worth trying to re-create that PR given the ClangTidy warnings with that new config?

I'll think more about a better solution long term. Maybe C++ modules fixes all of this.

Yeah, C++ modules definitely makes life easier. I thought of adding an issue or TODO to use modules in the future. But I'm wondering, How do we know that the time is right?

[jviotti]

How do we know that the time is right?

I think we'll know once most popular C++ libraries out there support it. So far I haven't seen even a single one. I think it will be a couple of years at least