In D47106#1110240, @probinson wrote:

(Replying here instead of inline as the thread is getting kind of long.)

Sorry, I didn't realize FileCheck would try to detect and explicitly diagnose reordering DAG across NOT. That seems like more trouble than it's worth. NOT is documented to enforce ordering, and we can make it just do that, and if you mess up then your test fails by not matching rather than telling you something is in the wrong order. (Then you can use -v or -vv to work out what happened, if you need to.)

Once we define the DAGs as separated into distinct groups by a NOT, I think it's conceptually straightforward to say each DAG group has a "matching range" from the start of the earliest match to the end of the latest match. Two DAG groups can't have overlapping match ranges (just like you are making individual DAG matches not overlap).

This splits the test case, as requested.

In D47114#1109821, @probinson wrote:

LGTM

If this doesn't accomplish the changes you had in mind, please let me know.

In D46919#1107296, @rsmith wrote:

In D46919#1101268, @jdenny wrote:

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

I don't think it's worth worrying about. We don't guarantee that the values round-trip in general (most of them are unsigneds being written to a bool, so we don't preserve values that are neither 0 nor 1).

Fixed some minor typos.

In D46919#1101268, @jdenny wrote:

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

Made a stab at the suggested changes.

Ping.

Made the suggested change. Thanks!

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

After further thought, this patch doesn't seem worthwhile. Sorry for all the noise.

I occurs to me now that I could have fixed __single_inheritance by declaring its argument "fake". However, I still prefer the above solution because (1) it should help with any other attributes where arguments should have been declared fake (but no, I haven't identified any yet), and (2) it generally makes attribute printing more succinct, including cases where originally implicit values were printing as explicit. Again, the sacrifice is that some explicit values now print as implicit, but at least the semantics don't change.

In D45465#1098710, @rsmith wrote:

Looks good, thanks.

Made the suggested change. Thanks!

Rebased. Ping.

Ping.

In D45463#1096629, @rsmith wrote:

In D45463#1096499, @jdenny wrote:

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

I think we should deprecate that flag, and change it to have no effect. It is an implementation detail of the AST printer and should never have been exposed by libclang in the first place.

In D45463#1096406, @rsmith wrote:

LGTM, thanks!

Made the suggested changes. Thanks.

I've implemented the suggestion to use ElaboratedType. See the last paragraph of the revised summary for details.

Made the suggested changes.

In D44598#1091098, @jmorse wrote:

Works perfectly for me on Win10 / cmd.exe, many thanks for getting that working!

This fixes windows 10 with cmd.exe for me. If anyone else wants to try it out, that would be great, but it should be ready for a patch review.

In D45093#1090292, @rsmith wrote:

If you want to force a particular printing policy to be used for -ast-print, I think it would be better to change the print call in lib/Frontend/ASTConsumers.cpp to pass your desired printing policy, rather than changing other components to prevent them from changing the ASTContext's default printing policy.

In D45093#1087652, @aaron.ballman wrote:

This approach generally looks good to me, but I'd like @rsmith's opinion on whether we should be trying to make -ast-print have good source fidelity or not. I was under the impression we wanted -ast-print to faithfully reproduce code at least as a low priority desire, but it sounds like it may only be intended as an approximation of the user's source code, so adding extra machinery to support better fidelity may be more maintenance burden than it's worth.

In D45463#1087174, @rsmith wrote:

TagSpecifierAs expands sizeof(PrintingPolicy) from 8 to 16 bytes. My concern is the comments on PrintingPolicy about how PrintingPolicy is intended to be small. My guess it that 16 bytes is still small enough, but perhaps Richard Smith, who wrote that comment, knows better.

This seems fine. See r270009 for some background for that comment -- we used to store a copy of the LangOptions in the PrintingPolicy (among other things), and copying these objects (including the potentially-large vectors within the LangOptions) was a double-digit percentage of the compile time of some compilations. That's a very different ball park from a change from 8 bytes to 16 bytes.

Rebased. Ping.

Rebased. Added example to summary. Ping.

Fixed test failures for windows cmd.exe, hopefully....

In D44598#1084413, @stella.stamenova wrote:

In D44598#1084281, @jdenny wrote:

I've attempted to replace the ":" command with "echo > null" in the case of windows cmd.exe. I don't build on windows, so someone else will have to try it, or we can let the buildbots tell us if windows is happy now.

The tests work correctly on our setup (Windows 10/Python 3).

In D44598#1085092, @jmorse wrote:

Aaannnddd, I didn't hit save in phabricator sorry, so here are those inline comments.

In D44598#1084413, @stella.stamenova wrote:

In D44598#1084281, @jdenny wrote:

I've attempted to replace the ":" command with "echo > null" in the case of windows cmd.exe. I don't build on windows, so someone else will have to try it, or we can let the buildbots tell us if windows is happy now.