Improve member documentation

Disable brace initialization

Use brace initialization in named constructors

Use named constructors instead of an argument to PathPolicy, add unit tests.

Remove configurable CacheSuccess, rename CacheFailure to CacheStatFailure.

LGTM, nice.

LGTM

In D145098#4167163, @Jake-Egan wrote:

Hi, this new test fails on AIX, could you take a look please?
https://lab.llvm.org/buildbot/#/builders/214/builds/6148/steps/6/logs/FAIL__Clang__modules-full-output-tu-order_c

Add asserts, make FullDeps take NumInputs in the constructor.

In D143613#4115935, @benlangmuir wrote:

In D143613#4115857, @jansvoboda11 wrote:

LGTM, but would be nice to have a test for this.

Oh, I didn't realize we didn't have one already. Added.

LGTM, but would be nice to have a test for this.

Remove unused CDB file

LGTM, thanks!

Having this check is great! Left some nits in-line.

In D135128#4108588, @stella.stamenova wrote:

In D135128#4107816, @jansvoboda11 wrote:

@stella.stamenova ping

Thanks for the reminder!

I spent some time looking for a combination of VS + Win 10 SDK that would work with the new flag, and I believe I've found (at least one). It looks like versions of the Win 10 SDK prior to 20348 do not work, but 20348 works fine in local testing with the latest versions of VS 2019 -- some newer versions of VS have a crash in some of the mlir tests, so I needed to find the right version there too.

Long story short, I believe VS 2019 16.11.23 with Win 10 SDK 10.0.20348.0 should work fine based on some local tests. I've updated the bot to these versions and I'd like to give it a day or so to make sure something else didn't break. After that, the new flag should work.

@stella.stamenova ping

In general, I think this approach makes sense.

LGTM

LGTM

Thanks for the patch. Left a couple of small suggestions.

Sounds like the easies way out is serializing all FileEntryRef objects we know in deterministic order.

LGTM.

Yes, I meant the pp-trace docs. Why would "we didn't find the included file" not be a traceable event?

LGTM.

Also I think we should mention this API in our docs, right where the removed original used to be.

LGTM with improved wording.

Fix test on Windows

In D142196#4068428, @Hahnfeld wrote:

I just checked and this "simplified" version of the callback would give us enough information to implement all we need in ROOT (plus some, the bool return value is nicer than temporarily playing with SuppressIncludeNotFoundError). For an upstream test, I could implement a unit test in clang/unittests/Lex/PPCallbacksTest.cpp or a "toy" tooling where #include <file.h?> is ignored if file.h doesn't exist? What do you think (assuming the general idea of adding this callback is fine)?

LGTM.

LGTM.

LGTM provided my comments get resolved, but I want @Bigcheese to look over this patch too.

LGTM.

If the plan is to eventually upstream that part of Cling, I'm fine with re-adding a safe version of this API.

In D119708#4059254, @Hahnfeld wrote:

Hello, sorry for the late heads-up, but this functionality is used by ROOT: https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282 Any chance of bringing this back?

Fix test on Windows(?), add comment

In D141450#4049049, @dblaikie wrote:

Perhaps I've got things confused, but my understanding of LSV was that it prevented other headers in the same modulemap from leaking into the use/inclusion of one header in a module. But that indirect inclusions were still valid/exposed (eg: header A, B, and C in a module, A includes C - without LSV, including A also incidentally exposes B, but with or without LSV including A does expose C).