- User Since
- Jul 7 2012, 2:55 PM (259 w, 14 h)
Fri, Jun 23
Adding folks interested in the indexing discussion.
Thu, Jun 22
General direction looks good.
LG. When you submit, please note the other fixes in the commit message (or make it 2 commits)
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?
Wed, Jun 21
Reviewing this mainly from the API view, and leaving the technical details to others :)
Tue, Jun 20
oh, and lg from my side
Mon, Jun 19
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.
Wed, Jun 14
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?
Generally LG from my side.
Mon, Jun 12
Wed, Jun 7
Fri, Jun 2
lg; sorry for the delay in review
Wed, May 31
Apart from fixme LG.
Tue, May 30
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).
May 23 2017
May 12 2017
May 11 2017
May 5 2017
Is there a specific reason to take this out? It seems generally useful to allow compilation-db implementors to provide sources.
May 3 2017
Apr 13 2017
Mar 9 2017
Mar 8 2017
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.
Mar 6 2017
Mar 2 2017
Feb 28 2017
Feb 27 2017
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?
Feb 23 2017
Feb 22 2017
Thanks. Landed in r295818
Thanks, landed in r295814.
Feb 20 2017
Feb 16 2017
Feb 15 2017
Feb 14 2017
Feb 8 2017
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.
Feb 7 2017
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 :)
Feb 3 2017
Jan 31 2017
lg minus adding the FIXME to the place where we need the change.
Generally looks like the right direction, minus that I'm not sure yet what the plan for things broken in BreakableToken are.
Jan 30 2017
Jan 25 2017
Jan 24 2017
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.
Jan 23 2017
Jan 20 2017
Jan 17 2017
Adding Jens as reviewer, as this is quite a bit of elisp :)