Adding folks interested in the indexing discussion.

In D34304#788080, @saugustine wrote:

In D34304#787699, @klimek wrote:

I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate?

This gets back to why the original patch plumbed the boolean all the way down to newInvocation.

newInvocation is the function that actually discards the dependency file options--today unconditionally.

General direction looks good.

LG. When you submit, please note the other fixes in the commit message (or make it 2 commits)

LG

LG

I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate?

lg

lg

Reviewing this mainly from the API view, and leaving the technical details to others :)

oh, and lg from my side

In D34263#784168, @akyrtzi wrote:

In D34263#783694, @klimek wrote:

how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang.

There is no other patch for using this preprocessor option. Any related improvements down the road will be about general improvements for error recovery, for example things like this:

• Introduce an UnresolvedTypename type instead of changing unresolved types to int.
• For @interace A : B, don't completely drop B from the super-class list of A if it is unresolved. These kind of improvements are not conditional.

In D34304#784496, @saugustine wrote:

In D34304#783675, @klimek wrote:

I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that.

This argument adapter would have to be passed down in a similar way, no?

buildASTFromCodeWithArgs, toolIinvocation::Run, and newInvocation are all entry-points that would need this behavior, and all are called by themselves in one place or another.

Generally this patch lg from my side - how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang.
When I saw your first patch my reaction was "wow, this works with this little change - cool".
With this patch it's "well, that doesn't seem to add too much complexity, so looks good".
I don't want to realize 5 patches down the line that we're sprinkling all of clang with single file conditional code without some discussion about the strategy with Richard.

I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that.

Ok, now I get it - can you please add tests? This is usually tested by adding a c-index-test based test.

Can you give a bit more background what this is trying to do?

LG

Generally LG from my side.

In D32480#773807, @Typz wrote:

So how do I proceed?

1. Keep the CompactNamespace option, and make "compacted" namespaces always add at most one level of indentation
2. Or assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

lg

lg; sorry for the delay in review

Apart from fixme LG.

I'm less concerned about everything suddenly re-indenting when you change code - if you use any kind of namespace indentation, that's what will happen now and then (and is why many style guides do not indent in namespaces).

LG

lg

Is there a specific reason to take this out? It seems generally useful to allow compilation-db implementors to provide sources.

Lg

In D31992#725912, @malaperle-ericsson wrote:

In D31992#725866, @krasimir wrote:

Seems that we're starting to hit some YAML/JSON mismatches, or is it that your YAML string support is lacking?

I don't think so. It seems like JSON and YAML are not completely aligned on escaped characters. See http://yaml.org/spec/1.2/spec.html#id2776092 which specifies unicode 8, 16, and 32 bits are escaped with \x, \u, and \U whereas http://www.json.org/string.gif specifies that unicode 16 bit characters can be encoded with \u but \x and \U are not supported. This leads me to believe that a YAML parser can read JSON but a JSON parser will not necessarily be able to read YAML. I thought about using json cpp but that's a much bigger change

lg

In D30650#693165, @Eugene.Zelenko wrote:

I think we should refactor this check as part of Static Analyzer, since it's path-sensitive.

Is the diff view on phab broken, or am I missing something? I only see a single line of diff now, and don't see a way to change the diff.

lg

lg

Also, do we want to not call ReplaceFile in Path.inc on Win if it potentially leaves temp files lying around?
Reading the code, it looks like we currently actually believe it may fail, and retry with MoveFileEx afterwards...

Can we open the files with FILE_SHARE_DELETE instead?

lg

Thanks. Landed in r295818

lg

Thanks, landed in r295814.

lg

lg

I'd probably still call it consumeComments or something, as we require a flush afterwards for the unconsumed comments, but I don't feel too strongly about that.

lg

I think this looks pretty good. More comments would help :) Also, organize is spelled with a 'z' in American.

+1 to "format only current document but save all" not making much sense :)

LG. Couple of questions.

lg minus adding the FIXME to the place where we need the change.

lg

Generally looks like the right direction, minus that I'm not sure yet what the plan for things broken in BreakableToken are.

lg

LG

This is starting to be pretty awesome. The one thing that would help me review the gist of the implementation a bit more is if that had a bit more comments. Perhaps go over the math in the code and put some comments in why we're doing what at various steps.

lg; thanks!

Adding Jens as reviewer, as this is quite a bit of elisp :)